Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2792643002:
Don't inherit visibility on the middle anonymous block in continuations (Closed)
|
Created:
3 years, 8 months ago by Gleb Lanbin Modified:
3 years, 8 months ago CC:
chromium-reviews, pdr+renderingwatchlist_chromium.org, zoltan1, blink-reviews-layout_chromium.org, szager+layoutwatch_chromium.org, eae+blinkwatch, leviw+renderwatch, jchaffraix+rendering, blink-reviews Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionDon't inherit visibility on the middle anonymous block in continuations
This is a follow-up patch on crrev.com/359399 that prevents using
containing block's visibility on the middle anonymous block in
continuations to avoid cases like
<a href="#" class="visibility:hidden">
<div>Inside a</div>
<a>
BUG=706324
Patch Set 1 #
Messages
Total messages: 18 (7 generated)
glebl@chromium.org changed reviewers: + cbiesinger@chromium.org
The CQ bit was checked by glebl@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
mstensho@opera.com changed reviewers: + mstensho@opera.com
Needs a test. Also I'm wondering what's so special about the visibility property (that warrants handling it specifically), and also why we manage to paint it correctly but not hit-test it correctly.
On 2017/03/31 19:42:38, mstensho wrote: > Needs a test. > > Also I'm wondering what's so special about the visibility property (that > warrants handling it specifically), and also why we manage to paint it correctly > but not hit-test it correctly. I think this boils down to the fact that this anonymous block that contains all DOM block children of the inline (the one typically in the middle of a continuation) shouldn't exist at all! Or at least be completely transparent and have no effect at all. I don't think there's one single object from which you can inherit all inheritable properties and get it right in all cases. In some cases it seems reasonable to inherit from the inline (i.e. the parent of the block children that are to be inserted into the anonymous block) (e.g. for "visibility"), while in other cases it's better to inherit from the containing block (e.g. for properties that have no effects on inlines, such as "direction" when "unicode-bidi" is "normal"). That makes me think that crrev.com/359399 shouldn't really be necessary (it's not really wrong, but it's attacking a problem that shouldn't exist). I'm thinking that the correct solution would be to make sure that this special anonymous block (I assume that it would be a lot of work and years of regressions to just get rid of it) is sufficiently ignored by consumers of the layout tree (such as painting and hit testing). In this particular case it looks like the anonymous block is considered a part of the inline as far as hit-testing is concerned, and that seems wrong.
mstensho@opera.com changed reviewers: + robhogan@gmail.com
Rob, you wrote the blamed CL. Any opinions or thoughts?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/03/31 at 20:35:27, mstensho wrote:
> Rob, you wrote the blamed CL. Any opinions or thoughts?
Yes, my CL looks wrong to me now.
I don't understand why I thought #block should inherit the direction property
from #rtl rather than #inline-parent in:
<!DOCTYPE html>
<style>
body {
margin: 0px;
padding: 0px;
}
</style>
<div style="position:absolute; width: 90px; height: 100px; background-color:red;
top: 10px; z-index: -1;"></div>
<div id="rtl" style="direction: rtl; width: 100px;">
<span id="inline-parent" style="direction: ltr">
<span id="block" style="display: block; height: 100px; width: 88px;
margin: 10px; border: 1px solid green; background-color:green;"></span>
</span>
</div>
bzbarsky implies it should in bug 286660 and that it's a containing block
problem, but I'm not seeing why that should be the case - direction is inherited
from parents not containing blocks and I don't see anything in the specs about
needing to layout out block-flow elements in the direction specified by their
containing block rather than their parent.
I get that direction:ltr has no actual effect on #inline-parent - there's
nothing layout can do with it unless it has something like
unicode-bidi:bidi-override. But that doesn't mean block-flow children of
#inline-parent shouldn't inherit that direction property and apply it to the
themselves though right? Hard to see how they would be expected to leap-frog to
the containing block in order to find a direction to inherit.
So I think our old result for the above test was correct and I should just
revert http://crrev.com/359399. Is there another side to the story though?
On 2017/04/01 20:54:34, rhogan wrote:
> On 2017/03/31 at 20:35:27, mstensho wrote:
> > Rob, you wrote the blamed CL. Any opinions or thoughts?
>
> Yes, my CL looks wrong to me now.
>
> I don't understand why I thought #block should inherit the direction property
> from #rtl rather than #inline-parent in:
>
> <!DOCTYPE html>
> <style>
> body {
> margin: 0px;
> padding: 0px;
> }
> </style>
> <div style="position:absolute; width: 90px; height: 100px;
background-color:red;
> top: 10px; z-index: -1;"></div>
> <div id="rtl" style="direction: rtl; width: 100px;">
> <span id="inline-parent" style="direction: ltr">
> <span id="block" style="display: block; height: 100px; width: 88px;
> margin: 10px; border: 1px solid green; background-color:green;"></span>
> </span>
> </div>
>
> bzbarsky implies it should in bug 286660 and that it's a containing block
> problem, but I'm not seeing why that should be the case - direction is
inherited
> from parents not containing blocks and I don't see anything in the specs about
> needing to layout out block-flow elements in the direction specified by their
> containing block rather than their parent.
>
> I get that direction:ltr has no actual effect on #inline-parent - there's
> nothing layout can do with it unless it has something like
> unicode-bidi:bidi-override. But that doesn't mean block-flow children of
> #inline-parent shouldn't inherit that direction property and apply it to the
> themselves though right? Hard to see how they would be expected to leap-frog
to
> the containing block in order to find a direction to inherit.
>
>
> So I think our old result for the above test was correct and I should just
> revert http://crrev.com/359399. Is there another side to the story though?
If you revert that patch, the need for this CL would disappear, yes.
As far as the "story" is concerned, my point was that your patch shouldn't
really be necessary (I wouldn't call the CL "right" or "wrong", but by right it
should be unnecessary - and if it is indeed unnecessary, that's great, and we
can revert it).
This anonymous block doesn't exist in the spec.
https://drafts.csswg.org/css2/visuren.html#anonymous-block-level
"When an inline box contains an in-flow block-level box, the inline box (and its
inline ancestors within the same line box) is broken around the block-level box
(and any block-level siblings that are consecutive or separated only by
collapsible whitespace and/or out-of-flow elements), splitting the inline box
into two boxes (even if either side is empty), one on each side of the
block-level box(es). The line boxes before the break and after the break are
enclosed in anonymous block boxes, and the block-level box becomes a sibling of
those anonymous boxes. When such an inline box is affected by relative
positioning, any resulting translation also affects the block-level box
contained in the inline box."
Blink does all that, and one more thing: wraps the block(s) inside an anonymous
block. That's an implementation detail, and no web-exposed API (including
painting and hit-testing) should leak that detail.
Fun-fact about Presto: Presto never creates anonymous block wrappers. Inline
boxes and block boxes may very well be siblings in the "layout tree" (Presto
doesn't even have a layout tree; there's only one tree, and that's the DOM node
tree). So the distinction between a block having "block children" vs. having
"inline children" doesn't exist. And this doesn't even violate any part of the
spec, since there's no way to observe this detail, neither with painting /
hit-testing, nor with scripts.
On 2017/04/03 at 11:01:48, mstensho wrote: > Blink does all that, and one more thing: wraps the block(s) inside an anonymous block. That's an implementation detail, and no web-exposed API (including painting and hit-testing) should leak that detail. Right, and my point is that the block(s) should inherit the direction from their inline parent, so should render https://chromium.googlesource.com/chromium/src/+/master/third_party/WebKit/La... with a margin of 10px on the right-hand side, not the left. If you agree with that then I should definitely revert. Or do you think we should render it the same way we do now but for some other reason?
On 2017/04/03 16:56:58, rhogan wrote: > On 2017/04/03 at 11:01:48, mstensho wrote: > > Blink does all that, and one more thing: wraps the block(s) inside an > anonymous block. That's an implementation detail, and no web-exposed API > (including painting and hit-testing) should leak that detail. > > Right, and my point is that the block(s) should inherit the direction from their > inline parent, so should render > https://chromium.googlesource.com/chromium/src/+/master/third_party/WebKit/La... > with a margin of 10px on the right-hand side, not the left. > > If you agree with that then I should definitely revert. Or do you think we > should render it the same way we do now but for some other reason? Hmm.. reverting it would make us identical to WebKit again, but at the same time make us diverge from all other engines. Test: http://bitten.servebeer.com/tmp/ltr-rtl-continuation.html https://developer.microsoft.com/en-us/microsoft-edge/tools/screenshots/?url=h... So I'll just repeat myself and say that this extra anonymous block that Blink and WebKit inject just gets in the way! :) <p>The hotpink box should be left-aligned within the yellow box. The yellow box should be right-aligned within the blue box.</p> <div id="outerBlock" style="direction:rtl; width:100px; height:100px; background:blue;"> <span id="inline" style="direction:ltr"> <div id="middleBlock" style="width:50px; height:50px; background:yellow;"> <div id="innerBlock" style="width:25px; height:25px; background:hotpink;"></div> </div> </span> </div> The containing block of #middleBlock is #outerBlock. So #middleBlock should be laid out in #outerBlock, which is RTL. #middleBlock itself is LTR (so that's how it will lay out its *children*), because it inherits from #inline. Because of the continuation machinery in Blink / WebKit, however, we rather get a layout tree based on this: <div id="outerBlock" style="direction:rtl; width:100px; height:100px; background:blue;"> <div id="anonBefore"> <span id="inline" style="direction:ltr"></span> </div> <div id="iCanHasAllTheBlocks"> <div id="middleBlock" style="width:50px; height:50px; background:yellow;"> <div id="innerBlock" style="width:25px; height:25px; background:hotpink;"></div> </div> </div> <div id="anonAfter"> <span id="inlineHelloItsMeAgain" style="direction:ltr"></span> </div> </div> Now, should #iCanHasAllTheBlocks inherit from #inline, or should it inherit from #outerBlock? Your CL from 2015 makes it inherit from #outerBlock. That's a good idea for "direction" (and it's also pretty, since it then gets the same style as its #anonBefore and #anonAfter sibling blocks), but it causes trouble for "visibility". So reverting your CL doesn't sound like the right solution. We either need to exclude this extra anonymous blocks from hit-testing and possibly other operations, or accept this new CL as is (i.e. allow the code in LayoutInline::addChildIgnoringContinuation() to inherit from multiple parents, depending on which property we're dealing with)
On 2017/04/03 at 20:08:39, mstensho wrote:
> The containing block of #middleBlock is #outerBlock. So #middleBlock should be
laid out in #outerBlock, which is RTL. #middleBlock itself is LTR (so that's how
it will lay out its *children*), because it inherits from #inline. Because of
the continuation machinery in Blink / WebKit, however, we rather get a layout
tree based on this:
>
>
> Now, should #iCanHasAllTheBlocks inherit from #inline, or should it inherit
from #outerBlock? Your CL from 2015 makes it inherit from #outerBlock. That's a
good idea for "direction" (and it's also pretty, since it then gets the same
style as its #anonBefore and #anonAfter sibling blocks), but it causes trouble
for "visibility".
>
Ah OK, I'm with you now. For the purposes of establishing its direction,
middleBlock's containing block is outerBlock and not iCanHasAllTheBlocks. So we
could either fix up determineLogicalLeftPosition & co. to refer to the child's
"correct" containing block's direction or since 'direction' really feels like
the special case do the inverse of the patch here and apply the inline's style
to the middleBlock wrapper but apply outerBlock's direction as a special case.
What makes 'direction' special here is that it's an attribute an element applies
to its children in layout rather than something it applies to itself.
Maybe another one we would need to special case is writing-mode. Though if you
try:
<!DOCTYPE html>
<div style="writing-mode:vertical-rl;">
<span style="writing:mode:horizontal-tb;">
<span style="display: block; width: 100px; height: 50px;
background-color:green;"></span>
<span style="display: block; width: 100px; height: 50px;
background-color:green;"></span>
</span>
</div>
It already works, even with my patch reverted - the anonymous block gets
vertical-rl as it's writing mode style rather than the inline's writing-mode. So
the inheritance of writing-mode is managed differently to the inheritance for
direction it seems. If so, then that's what we want to emulate I guess.
Interesting, I wonder how it does that..
On 2017/04/03 at 23:12:38, rhogan wrote: > On 2017/04/03 at 20:08:39, mstensho wrote: > > The containing block of #middleBlock is #outerBlock. So #middleBlock should be laid out in #outerBlock, which is RTL. #middleBlock itself is LTR (so that's how it will lay out its *children*), because it inherits from #inline. Because of the continuation machinery in Blink / WebKit, however, we rather get a layout tree based on this: > > > > > > Now, should #iCanHasAllTheBlocks inherit from #inline, or should it inherit from #outerBlock? Your CL from 2015 makes it inherit from #outerBlock. That's a good idea for "direction" (and it's also pretty, since it then gets the same style as its #anonBefore and #anonAfter sibling blocks), but it causes trouble for "visibility". > > > > Ah OK, I'm with you now. For the purposes of establishing its direction, middleBlock's containing block is outerBlock and not iCanHasAllTheBlocks. So we could either fix up determineLogicalLeftPosition & co. to refer to the child's "correct" containing block's direction or since 'direction' really feels like the special case do the inverse of the patch here and apply the inline's style to the middleBlock wrapper but apply outerBlock's direction as a special case. What makes 'direction' special here is that it's an attribute an element applies to its children in layout rather than something it applies to itself. > > Maybe another one we would need to special case is writing-mode. Though if you try: > > <!DOCTYPE html> > <div style="writing-mode:vertical-rl;"> > <span style="writing:mode:horizontal-tb;"> > <span style="display: block; width: 100px; height: 50px; background-color:green;"></span> > <span style="display: block; width: 100px; height: 50px; background-color:green;"></span> > </span> > </div> > > It already works, even with my patch reverted - the anonymous block gets vertical-rl as it's writing mode style rather than the inline's writing-mode. So the inheritance of writing-mode is managed differently to the inheritance for direction it seems. If so, then that's what we want to emulate I guess. Interesting, I wonder how it does that.. Ach, never mind - a typo in "writing:mode:horizontal-tb;" - the writing-mode makes a formatting context of the span, which turns it into a block so the problem doesn't arise for writing modes. So, it may be that we only need to worry about 'direction' here.
Closing this. Superseded by https://codereview.chromium.org/2806123002/ |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
