Bug 57459 – REGRESSION(r82419): extra border in t0805-c5519-ibrdr-r-00-a.html

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Yael]

An extra border is visible in t0805-c5519-ibrdr-r-00-a.html .
This is only in the Qt port.

[Csaba Osztrogonác]

Could you add the test to the Skipped list until fix?

[Csaba Osztrogonác]

I added it to the skipped list: http://trac.webkit.org/changeset/82476

[Yael]

I have a fix for this, but since I am away all of next week (with no internet :) ,
I will upload it when I get back home.
Sorry for the inconvenience.

[Yael]

Created attachment 88938 [details]
Patch.

Slightly change the way we decide if a InlineFlowBox is the last box for its renderer.
We include the border in that box under these conditions:
(1) The next line was not created, or it is constructed. We check the previous line for rtl.
(2) The logicallyLastRun is not a descendant of this renderer.
(3) The logicallyLastRun is a descendant of this renderer, but it is the last child of this renderer and it does not wrap to the next line.
I added the tests from bug 47210, because now the draw the borders correctly.

More cleanup patches will follow.

[WebKit Review Bot]

Attachment 88938 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8379423

[Early Warning System Bot]

Attachment 88938 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8381342

[Yael]

Created attachment 88939 [details]
Patch.

Fix the build. Not sure why it did not fail for me, but only on the buildbot.

[Eric Seidel (no email)]

Comment on attachment 88939 [details]
Patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=88939&action=review

> Source/WebCore/rendering/InlineFlowBox.cpp:265
> +    while (parent && renderer()->containingBlock() != parent) {

renderer()->containingBlock() does a lot of work, no?  Seems we should cache that.

I'm also not sure I understand this part of the change.  The rest of the change looks fine.

[Yael]

(In reply to comment #8)
> (From update of attachment 88939 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=88939&action=review
> 
> > Source/WebCore/rendering/InlineFlowBox.cpp:265
> > +    while (parent && renderer()->containingBlock() != parent) {
> 
> renderer()->containingBlock() does a lot of work, no?  Seems we should cache that.
> 
> I'm also not sure I understand this part of the change.  The rest of the change looks fine.

Thank you for the review,
I'll change the code to cache the containing block. The reason for this change is that one of the pixel tests was failing on something like
<div><span><div style="display:inline-block"><span></span></div></span></div>.

Without this change, we would break out of the while loop when we see the div with display inline-block, but we should not.

[Eric Seidel (no email)]

I see.  Makes sense.  But we should still cache the containingBlock() because recomputing it each time through the loop is expensive.

[Yael]

(In reply to comment #10)
> I see.  Makes sense.  But we should still cache the containingBlock() because recomputing it each time through the loop is expensive.

Agree, I am working on it :)

[Yael]

Created attachment 89054 [details]
Patch.

Add caching of containingBlock() so that we don't calculate it repeatedly inside the while loop.

[Eric Seidel (no email)]

Comment on attachment 89054 [details]
Patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=89054&action=review

> Source/WebCore/rendering/InlineFlowBox.cpp:266
> +    while (parent && block != parent) {

Hmmm.  The previous check would walk up the parent chain until the first block.  This one is now looking for a specific block, the containing block of the renderer corresponding to this flow.

Seems we'll end up walking a lot more in cases where we previously would have terminated at the first block (if curr is not a child of renderer()).

Are you sure this is intentional?  Are you just trying to handle the inline-block case?  Should we just tweak the previous algorithm to not stop at inlineBlock?  Or should this new one have a check which stops at any block, if it's not an inline block, regardless of whether we've matched the containing block or not?

I'm not sure.

[Yael]

(In reply to comment #13)
> (From update of attachment 89054 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=89054&action=review
> 
> > Source/WebCore/rendering/InlineFlowBox.cpp:266
> > +    while (parent && block != parent) {
> 
> Hmmm.  The previous check would walk up the parent chain until the first block.  This one is now looking for a specific block, the containing block of the renderer corresponding to this flow.
> 
> Seems we'll end up walking a lot more in cases where we previously would have terminated at the first block (if curr is not a child of renderer()).
> 
> Are you sure this is intentional?  Are you just trying to handle the inline-block case?  Should we just tweak the previous algorithm to not stop at inlineBlock?  Or should this new one have a check which stops at any block, if it's not an inline block, regardless of whether we've matched the containing block or not?
> 
> I'm not sure.

It is true that we are walking up the tree more now, in cases that curr is not a child of renderer() and it is a child of a block with display inlineBlock.
IMO the "optimization" that I removed was wrong, and caused bugs in cases that curr is a child of renderer(), and there was a inlineBlock in between.
We are not walking more now then before, in other cases.

Or did I misunderstand your concern? :)

[Yael]

(In reply to comment #14)
> (In reply to comment #13)
> > (From update of attachment 89054 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=89054&action=review
> > 
> > > Source/WebCore/rendering/InlineFlowBox.cpp:266
> > > +    while (parent && block != parent) {
> > 
> > Hmmm.  The previous check would walk up the parent chain until the first block.  This one is now looking for a specific block, the containing block of the renderer corresponding to this flow.
> > 
> > Seems we'll end up walking a lot more in cases where we previously would have terminated at the first block (if curr is not a child of renderer()).
> > 
> > Are you sure this is intentional?  Are you just trying to handle the inline-block case?  Should we just tweak the previous algorithm to not stop at inlineBlock?  Or should this new one have a check which stops at any block, if it's not an inline block, regardless of whether we've matched the containing block or not?
> > 
> > I'm not sure.
> 
> It is true that we are walking up the tree more now, in cases that curr is not a child of renderer() and it is a child of a block with display inlineBlock.
> IMO the "optimization" that I removed was wrong, and caused bugs in cases that curr is a child of renderer(), and there was a inlineBlock in between.
> We are not walking more now then before, in other cases.
> 
> Or did I misunderstand your concern? :)

Ok, I think I get it, please ignore my previous comment.

[Yael]

Created attachment 89140 [details]
Patch.

Changed the stop condition on the while loop to stop on the first block that is not an inline-block, instead of stopping on the containing block of renderer(), as suggested by comment #13.

[Eric Seidel (no email)]

Comment on attachment 89140 [details]
Patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=89140&action=review

> Source/WebCore/rendering/InlineFlowBox.cpp:265
> +    while (parent && (!parent->isRenderBlock() || parent->style()->display() == INLINE_BLOCK)) {

I thought there was an isInlineBlock() check on RenderObject?

[Yael]

(In reply to comment #17)
> (From update of attachment 89140 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=89140&action=review
> 
> > Source/WebCore/rendering/InlineFlowBox.cpp:265
> > +    while (parent && (!parent->isRenderBlock() || parent->style()->display() == INLINE_BLOCK)) {
> 
> I thought there was an isInlineBlock() check on RenderObject?

There is isInline() method. Calling it on a RenderBlock should essentially be the same as isInlineBlock(). A patch is coming.

[Yael]

Created attachment 89149 [details]
Patch.

Address comment #17.

[Eric Seidel (no email)]

Comment on attachment 89149 [details]
Patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=89149&action=review

> Source/WebCore/rendering/InlineFlowBox.cpp:269
> +        if (parent == renderer())
> +            return true;

Why does this now return true?  It used to return false.

[Eric Seidel (no email)]

Comment on attachment 89149 [details]
Patch.

I think the end result is that "onEndChain" never made any sense.  Now it makes a differnet amount of non-sense, but seems to have the right behavior. We should split it off into a static-inline with a nice name, near the one place it's used. I don't think it makes any sense on InlineFlowBox.  It also probably only makes sense to call it on RootInlineBoxes, no?  Your change seems fine though.  THanks for all the info.

[WebKit Commit Bot]

Comment on attachment 89149 [details]
Patch.

Clearing flags on attachment: 89149

Committed r83638: <http://trac.webkit.org/changeset/83638>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Yael]

Looks like css1/box_properties/padding_right_inline.html requires new results for Qt.
IMO the new results are correct and match what Firefox does for that test case.
I am going to update the results.

[Andrey Kosyakov]

mac/fast/borders/bidi-004-expected.png added here actually has the boxes mismatching, I wonder if this is this intended?

[Yael]

(In reply to comment #25)
> mac/fast/borders/bidi-004-expected.png added here actually has the boxes mismatching, I wonder if this is this intended?

Yes, it is intended , until https://bugs.webkit.org/show_bug.cgi?id=47210 is fixed.