Bug 215410 – [LFC][IFC] Finalize InlineBox alignment in LineBox

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[alan]

ssia

[alan]

Created attachment 406455 [details]
Patch

[alan]

Created attachment 406481 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/67398052>

[alan]

Created attachment 407560 [details]
Patch

[alan]

Created attachment 407618 [details]
Patch

[Antti Koivisto]

Comment on attachment 407618 [details]
Patch

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

> Source/WebCore/layout/inlineformatting/InlineFormattingContext.cpp:514
> -    // Compute box final geometry.
> +    // Compute final box geometry.

Solid progression.

> Source/WebCore/layout/inlineformatting/InlineLineBox.cpp:191
> +    auto lineIsConsideredEmpty = !lineHasImaginaryStrut || isLineVisuallyEmpty == IsLineVisuallyEmpty::Yes ? InlineBox::IsConsideredEmpty::Yes : InlineBox::IsConsideredEmpty::No;

For this sort of things bools would read better.

[alan]

(In reply to Antti Koivisto from comment #6)
> Comment on attachment 407618 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=407618&action=review
> 
> > Source/WebCore/layout/inlineformatting/InlineFormattingContext.cpp:514
> > -    // Compute box final geometry.
> > +    // Compute final box geometry.
> 
> Solid progression.
> 
> > Source/WebCore/layout/inlineformatting/InlineLineBox.cpp:191
> > +    auto lineIsConsideredEmpty = !lineHasImaginaryStrut || isLineVisuallyEmpty == IsLineVisuallyEmpty::Yes ? InlineBox::IsConsideredEmpty::Yes : InlineBox::IsConsideredEmpty::No;
> 
> For this sort of things bools would read better.
Yeah, agree. I'll look into this (in a separate patch).

[alan]

Created attachment 407850 [details]
Patch

[EWS]

Committed r266509: <https://trac.webkit.org/changeset/266509>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 407850 [details].

[Aakash Jain]

(In reply to EWS from comment #9)
> Committed r266509: <https://trac.webkit.org/changeset/266509>
Seems like this broke css2.1/t0905-c5525-fltwidth-00-c-g.html on ios-wk2. EWS also indicated that failure on previous version of this patch.

History: https://results.webkit.org/?suite=layout-tests&test=css2.1%2Ft0905-c5525-fltwidth-00-c-g.html

e.g.: https://build.webkit.org/results/Apple%20iOS%2013%20Simulator%20Release%20WK2%20(Tests)/r266509%20(6539)/results.html

-        RenderText {#text} at (392,0) size 784x519
+        RenderText {#text} at (392,0) size 785x519

-          text run at (745,360) width 39: "this is"
+          text run at (745,360) width 40: "this is"

[alan]

(In reply to Aakash Jain from comment #10)
> (In reply to EWS from comment #9)
> > Committed r266509: <https://trac.webkit.org/changeset/266509>
> Seems like this broke css2.1/t0905-c5525-fltwidth-00-c-g.html on ios-wk2.
> EWS also indicated that failure on previous version of this patch.
> 
> History:
> https://results.webkit.org/?suite=layout-tests&test=css2.1%2Ft0905-c5525-
> fltwidth-00-c-g.html
> 
> e.g.:
> https://build.webkit.org/results/
> Apple%20iOS%2013%20Simulator%20Release%20WK2%20(Tests)/r266509%20(6539)/
> results.html
> 
> -        RenderText {#text} at (392,0) size 784x519
> +        RenderText {#text} at (392,0) size 785x519
> 
> -          text run at (745,360) width 39: "this is"
> +          text run at (745,360) width 40: "this is"
Thanks. looking into it now.

[Aakash Jain]

Thanks. This is somewhat urgent as it is slowing down iOS-wk2 EWS.

[Aakash Jain]

Actually the test was already marked as failing in r266565. So, not so urgent, but would be good to fix soon anyways.

[alan]

(In reply to Aakash Jain from comment #13)
> Actually the test was already marked as failing in r266565. So, not so
> urgent, but would be good to fix soon anyways.
Yeah I noticed it too. I briefly looked and it seemed like a rounding issue in the output and does not affect functionality. Will investigate.

[alan]

(In reply to Aakash Jain from comment #13)
> Actually the test was already marked as failing in r266565. So, not so
> urgent, but would be good to fix soon anyways.
Yeah, so as I suspected this is an float arithmetic issue:

"This is a very unfortunate float arithmetic issue where the run's x position is now 0.000061px off -and the ceil() operation in "dump renderer as text" results in a different integral value ->
rect.x(745.766968) rect.width(38.233074) ceilf 785.000000
rect.x(745.766907) rect.width(38.233074) ceilf 784.000000"

will rebaseline.

[alan]

Created attachment 408188 [details]
Rebaseline patch

[alan]

reopen for the rebaseline patch.

[EWS]

Committed r266707: <https://trac.webkit.org/changeset/266707>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 408188 [details].