Bug 215410 - [LFC][IFC] Finalize InlineBox alignment in LineBox
Summary: [LFC][IFC] Finalize InlineBox alignment in LineBox
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: zalan
URL:
Keywords: InRadar
Depends on:
Blocks: 215752
  Show dependency treegraph
 
Reported: 2020-08-12 05:43 PDT by zalan
Modified: 2020-09-07 11:54 PDT (History)
9 users (show)

See Also:


Attachments
Patch (51.39 KB, patch)
2020-08-12 05:45 PDT, zalan
no flags Details | Formatted Diff | Diff
Patch (53.82 KB, patch)
2020-08-12 15:13 PDT, zalan
no flags Details | Formatted Diff | Diff
Patch (55.32 KB, patch)
2020-08-29 21:20 PDT, zalan
no flags Details | Formatted Diff | Diff
Patch (54.81 KB, patch)
2020-08-31 13:01 PDT, zalan
no flags Details | Formatted Diff | Diff
Patch (54.90 KB, patch)
2020-09-02 19:00 PDT, zalan
no flags Details | Formatted Diff | Diff
Rebaseline patch (3.21 KB, patch)
2020-09-07 11:28 PDT, zalan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description zalan 2020-08-12 05:43:53 PDT
ssia
Comment 1 zalan 2020-08-12 05:45:24 PDT
Created attachment 406455 [details]
Patch
Comment 2 zalan 2020-08-12 15:13:31 PDT
Created attachment 406481 [details]
Patch
Comment 3 Radar WebKit Bug Importer 2020-08-19 05:44:14 PDT
<rdar://problem/67398052>
Comment 4 zalan 2020-08-29 21:20:13 PDT
Created attachment 407560 [details]
Patch
Comment 5 zalan 2020-08-31 13:01:13 PDT
Created attachment 407618 [details]
Patch
Comment 6 Antti Koivisto 2020-09-02 08:07:20 PDT
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.
Comment 7 zalan 2020-09-02 09:46:09 PDT
(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).
Comment 8 zalan 2020-09-02 19:00:40 PDT
Created attachment 407850 [details]
Patch
Comment 9 EWS 2020-09-03 03:15:09 PDT
Committed r266509: <https://trac.webkit.org/changeset/266509>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 407850 [details].
Comment 10 Aakash Jain 2020-09-04 05:35:20 PDT
(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"
Comment 11 zalan 2020-09-04 05:37:58 PDT
(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.
Comment 12 Aakash Jain 2020-09-04 07:00:56 PDT
Thanks. This is somewhat urgent as it is slowing down iOS-wk2 EWS.
Comment 13 Aakash Jain 2020-09-04 07:25:37 PDT
Actually the test was already marked as failing in r266565. So, not so urgent, but would be good to fix soon anyways.
Comment 14 zalan 2020-09-04 07:35:43 PDT
(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.
Comment 15 zalan 2020-09-04 12:12:44 PDT
(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.
Comment 16 zalan 2020-09-07 11:28:10 PDT
Created attachment 408188 [details]
Rebaseline patch
Comment 17 zalan 2020-09-07 11:30:43 PDT
reopen for the rebaseline patch.
Comment 18 EWS 2020-09-07 11:54:28 PDT
Committed r266707: <https://trac.webkit.org/changeset/266707>

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