Skip to content

GitLab

T
tangled
Commit 1cb61534 authored by robhogan@gmail.com's avatar robhogan@gmail.com
Browse files
Options

Revert of Empty inline elements always get a line box. (https://codereview.chromium.org/300853007/) 

Reason for revert:
This changed the subpixel positioning of glyphs on lines with first-letter renderers on Mac builds. See https://storage.googleapis.com/chromium-layout-test-archives/WebKit_Mac10_6/28423/layout-test-results/results.html as an example.

I think the new results are benign - but would like to confirm with leviw and reland as NeedsRebaseline if we're both happy with them.


Original issue's description:
> Empty inline elements always get a line box.
> 
> CSS 2.1 (http://www.w3.org/TR/CSS2/visudet.html#leading) says: "Empty inline 
> elements generate empty inline boxes, but these boxes still have margins, padding, 
> borders and a line height, and thus influence [line-height calculation] just 
> like elements with content."
> 
> We weren't creating lineboxes for empty inline elements when they occurred
> between a single trailing space and a single leading space - this is because
> our linebox creation was based on the too-narrow premise that we were already
> ignoring spaces. However, if an empty inline element comes after a trailing 
> space but before one or more leading spaces then it will be ignored as part 
> of the collapsed space between the trailing space and the next non-collapsible 
> space - so we should force the creation of a linebox in such cases.
> 
> This allows us to pass Hixie's empty inline test: 
> http://www.hixie.ch/tests/evil/mixed/emptyinline.html
> 
> BUG=328939
> 
> Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=175205

TBR=leviw@chromium.org
NOTREECHECKS=true
NOTRY=true
BUG=328939

Review URL: https://codereview.chromium.org/302233002

git-svn-id: svn://svn.chromium.org/blink/trunk@175207 bbb929c8-8fbe-4397-9dbb-9b2b20218538
parent 16e7476a
Hide whitespace changes
Inline Side-by-side
Showing with 24 additions and 88 deletions
third_party/WebKit/LayoutTests/fast/inline/empty-inline-before-collapsed-space-expected.txt deleted 100644 → 0
View file @ 16e7476a
An empty inline should still get a linebox even when it precedes a space at which we start collapsing whitespace.
X X
PASS
X X
PASS
X X
PASS
X X
PASS
XX
PASS
X X
PASS
X X
PASS
third_party/WebKit/LayoutTests/fast/inline/empty-inline-before-collapsed-space.html deleted 100644 → 0
View file @ 16e7476a
<!DOCTYPE HTML>
<style>
#container {
font: 20px/1 Ahem;
border: solid orange;
}
</style>
<script src="../../resources/check-layout.js"></script>
<p>An empty inline should still get a linebox even when it precedes a space at which we start collapsing whitespace.</p>
<div id="container" style="width: 100px" data-expected-client-height=50>
X <span style="font-size: 50px;"></span> X
</div>
<div id="container" style="width: 100px" data-expected-client-height=50>
X<span style="font-size: 50px;"></span> X
</div>
<div id="container" style="width: 100px" data-expected-client-height=50>
X <span style="font-size: 50px;"></span>X
</div>
<div id="container" style="width: 100px" data-expected-client-height=50>
X <span style="font-size: 50px;"></span> X
</div>
<div id="container" style="width: 100px" data-expected-client-height=50>
X<span style="font-size: 50px;"></span>X
</div>
<div id="container" style="width: 100px" data-expected-client-height=50>
X<span style="font-size: 50px;"></span> X
</div>
<div id="container" style="width: 100px" data-expected-client-height=50>
X <span style="font-size: 50px;"></span> X
</div>
<script>
checkLayout('#container');
</script>
third_party/WebKit/Source/core/rendering/line/BreakingContext.cpp
View file @ 1cb61534
......@@ -48,7 +48,7 @@ InlineIterator BreakingContext::handleEndOfLine()
// Sanity check our midpoints.
m_lineMidpointState.checkMidpoints(m_lineBreak);
m_trailingObjects.updateMidpointsForTrailingObjects(m_lineMidpointState, m_lineBreak, TrailingObjects::CollapseFirstSpace);
m_trailingObjects.updateMidpointsForTrailingBoxes(m_lineMidpointState, m_lineBreak, TrailingObjects::CollapseFirstSpace);
// We might have made lineBreak an iterator that points past the end
// of the object. Do this adjustment to make it point to the start
......
third_party/WebKit/Source/core/rendering/line/BreakingContextInlineHeaders.h
View file @ 1cb61534
......@@ -362,7 +362,7 @@ inline void BreakingContext::handleOutOfFlowPositioned(Vector<RenderBox*>& posit
if (isInlineType || box->container()->isRenderInline()) {
if (m_ignoringSpaces)
m_lineMidpointState.ensureLineBoxInsideIgnoredSpaces(box);
m_trailingObjects.appendObjectIfNeeded(box);
m_trailingObjects.appendBoxIfNeeded(box);
} else {
positionedObjects.append(box);
}
......@@ -419,27 +419,25 @@ inline void BreakingContext::handleEmptyInline()
RenderInline* flowBox = toRenderInline(m_current.object());
// Now that some inline flows have line boxes, if we are already ignoring spaces, we need
// to make sure that we stop to include this object and then start ignoring spaces again.
// If this object is at the start of the line, we need to behave like list markers and
// start ignoring spaces.
bool requiresLineBox = alwaysRequiresLineBox(m_current.object());
if (requiresLineBox || requiresLineBoxForContent(flowBox, m_lineInfo)) {
// An empty inline that only has line-height, vertical-align or font-metrics will
// not force linebox creation (and thus affect the height of the line) if the rest of the line is empty.
// An empty inline that only has line-height, vertical-align or font-metrics will only get a
// line box to affect the height of the line if the rest of the line is not empty.
if (requiresLineBox)
m_lineInfo.setEmpty(false, m_block, &m_width);
if (m_ignoringSpaces) {
// If we are in a run of ignored spaces then ensure we get a linebox if lineboxes are eventually
// created for the line...
m_trailingObjects.clear();
m_lineMidpointState.ensureLineBoxInsideIgnoredSpaces(m_current.object());
} else if (m_blockStyle->collapseWhiteSpace() && m_resolver.position().object() == m_current.object()
&& shouldSkipWhitespaceAfterStartObject(m_block, m_current.object(), m_lineMidpointState)) {
// If this object is at the start of the line, we need to behave like list markers and
// start ignoring spaces.
// Like with list markers, we start ignoring spaces to make sure that any
// additional spaces we see will be discarded.
m_currentCharacterShouldCollapseIfPreWap = m_currentCharacterIsSpace = true;
m_ignoringSpaces = true;
} else {
// If we are after a trailing space but aren't ignoring spaces yet then ensure we get a linebox
// if we encounter collapsible whitepace.
m_trailingObjects.appendObjectIfNeeded(m_current.object());
}
}
......@@ -771,7 +769,7 @@ inline bool BreakingContext::handleText(WordMeasurements& wordMeasurements, bool
// spaces. Create a midpoint to terminate the run
// before the second space.
m_lineMidpointState.startIgnoringSpaces(m_startOfIgnoredSpaces);
m_trailingObjects.updateMidpointsForTrailingObjects(m_lineMidpointState, InlineIterator(), TrailingObjects::DoNotCollapseFirstSpace);
m_trailingObjects.updateMidpointsForTrailingBoxes(m_lineMidpointState, InlineIterator(), TrailingObjects::DoNotCollapseFirstSpace);
}
}
} else if (m_ignoringSpaces) {
......
third_party/WebKit/Source/core/rendering/line/TrailingObjects.cpp
View file @ 1cb61534
......@@ -28,7 +28,7 @@
namespace WebCore {
void TrailingObjects::updateMidpointsForTrailingObjects(LineMidpointState& lineMidpointState, const InlineIterator& lBreak, CollapseFirstSpaceOrNot collapseFirstSpace)
void TrailingObjects::updateMidpointsForTrailingBoxes(LineMidpointState& lineMidpointState, const InlineIterator& lBreak, CollapseFirstSpaceOrNot collapseFirstSpace)
{
if (!m_whitespace)
return;
......@@ -46,13 +46,13 @@ void TrailingObjects::updateMidpointsForTrailingObjects(LineMidpointState& lineM
// Now make sure every single trailingPositionedBox following the trailingSpaceMidpoint properly stops and starts
// ignoring spaces.
size_t currentMidpoint = trailingSpaceMidpoint + 1;
for (size_t i = 0; i < m_objects.size(); ++i) {
for (size_t i = 0; i < m_boxes.size(); ++i) {
if (currentMidpoint >= lineMidpointState.numMidpoints()) {
// We don't have a midpoint for this box yet.
lineMidpointState.ensureLineBoxInsideIgnoredSpaces(m_objects[i]);
lineMidpointState.ensureLineBoxInsideIgnoredSpaces(m_boxes[i]);
} else {
ASSERT(lineMidpointState.midpoints()[currentMidpoint].object() == m_objects[i]);
ASSERT(lineMidpointState.midpoints()[currentMidpoint + 1].object() == m_objects[i]);
ASSERT(lineMidpointState.midpoints()[currentMidpoint].object() == m_boxes[i]);
ASSERT(lineMidpointState.midpoints()[currentMidpoint + 1].object() == m_boxes[i]);
}
currentMidpoint += 2;
}
......@@ -64,8 +64,8 @@ void TrailingObjects::updateMidpointsForTrailingObjects(LineMidpointState& lineM
unsigned pos = length >= 2 ? length - 2 : UINT_MAX;
InlineIterator endMid(0, m_whitespace, pos);
lineMidpointState.startIgnoringSpaces(endMid);
for (size_t i = 0; i < m_objects.size(); ++i) {
lineMidpointState.ensureLineBoxInsideIgnoredSpaces(m_objects[i]);
for (size_t i = 0; i < m_boxes.size(); ++i) {
lineMidpointState.ensureLineBoxInsideIgnoredSpaces(m_boxes[i]);
}
}
}
......
third_party/WebKit/Source/core/rendering/line/TrailingObjects.h
View file @ 1cb61534
......@@ -29,7 +29,7 @@
namespace WebCore {
class InlineIterator;
class RenderObject;
class RenderBox;
class RenderText;
struct BidiRun;
......@@ -39,19 +39,6 @@ template <class Iterator> class MidpointState;
typedef BidiResolver<InlineIterator, BidiRun> InlineBidiResolver;
typedef MidpointState<InlineIterator> LineMidpointState;
// This class allows us to ensure lineboxes are created in the right place on the line when
// an out-of-flow positioned object or an empty inline is encountered between a trailing space
// and subsequent spaces and we want to ignore (i.e. collapse) surplus whitespace. So for example:
// <div>X <span></span> Y</div>
// or
// <div>X <div style="position: absolute"></div> Y</div>
// In both of the above snippets the inline and the positioned object occur after a trailing space
// and before a space that will cause our line breaking algorithm to start ignoring spaces. When it
// does that we want to ensure that the inline/positioned object gets a linebox and that it is part
// of the collapsed whitespace. So to achieve this we use appendObjectIfNeeded() to keep track of
// objects encountered after a trailing whitespace and updateMidpointsForTrailingObjects() to put
// them in the right place when we start ignoring surplus whitespace.
class TrailingObjects {
public:
TrailingObjects()
......@@ -71,22 +58,22 @@ public:
// Using resize(0) rather than clear() here saves 2% on
// PerformanceTests/Layout/line-layout.html because we avoid freeing and
// re-allocating the underlying buffer repeatedly.
m_objects.resize(0);
m_boxes.resize(0);
}
void appendObjectIfNeeded(RenderObject* object)
void appendBoxIfNeeded(RenderBox* box)
{
if (m_whitespace)
m_objects.append(object);
m_boxes.append(box);
}
enum CollapseFirstSpaceOrNot { DoNotCollapseFirstSpace, CollapseFirstSpace };
void updateMidpointsForTrailingObjects(LineMidpointState&, const InlineIterator& lBreak, CollapseFirstSpaceOrNot);
void updateMidpointsForTrailingBoxes(LineMidpointState&, const InlineIterator& lBreak, CollapseFirstSpaceOrNot);
private:
RenderText* m_whitespace;
Vector<RenderObject*, 4> m_objects;
Vector<RenderBox*, 4> m_boxes;
};
}
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment