WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
221337
Fix replaced element definiteness as a grid-item
https://bugs.webkit.org/show_bug.cgi?id=221337
Summary
Fix replaced element definiteness as a grid-item
zsun
Reported
2021-02-03 09:15:39 PST
percentage-size-indefinite-replaced.html in WPT fails for Safari
Attachments
Patch
(5.91 KB, patch)
2021-02-05 07:56 PST
,
zsun
no flags
Details
Formatted Diff
Diff
Patch
(11.66 KB, patch)
2021-02-16 06:20 PST
,
zsun
no flags
Details
Formatted Diff
Diff
Patch
(12.52 KB, patch)
2021-02-23 08:05 PST
,
zsun
no flags
Details
Formatted Diff
Diff
Patch
(11.67 KB, patch)
2021-02-25 06:12 PST
,
zsun
no flags
Details
Formatted Diff
Diff
Patch
(11.63 KB, patch)
2021-02-26 07:21 PST
,
zsun
no flags
Details
Formatted Diff
Diff
Show Obsolete
(4)
View All
Add attachment
proposed patch, testcase, etc.
zsun
Comment 1
2021-02-05 07:56:15 PST
Created
attachment 419408
[details]
Patch
Radar WebKit Bug Importer
Comment 2
2021-02-10 09:16:13 PST
<
rdar://problem/74191286
>
Javier Fernandez
Comment 3
2021-02-15 13:52:10 PST
Comment on
attachment 419408
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=419408&action=review
> Source/WebCore/ChangeLog:9 > + has an indefinite containing-block override logical-height.
I think it'd be useful to know why we need to return false for replaced grid items. Perhaps an excerpt of the spec and a link to the section where it's stated.
> Source/WebCore/rendering/RenderBoxModelObject.cpp:285 > + return thisBox->overridingContainingBlockContentLogicalHeight() == WTF::nullopt;
I don't understand why this change affects only to replaced grid items. Could you please elaborate on this ?
zsun
Comment 4
2021-02-16 06:20:54 PST
Created
attachment 420464
[details]
Patch
zsun
Comment 5
2021-02-16 06:53:23 PST
(In reply to Javier Fernandez from
comment #3
)
> Comment on
attachment 419408
[details]
> Patch > > View in context: >
https://bugs.webkit.org/attachment.cgi?id=419408&action=review
> > > Source/WebCore/ChangeLog:9 > > + has an indefinite containing-block override logical-height. > > I think it'd be useful to know why we need to return false for replaced grid > items. Perhaps an excerpt of the spec and a link to the section where it's > stated. >
Updated.
> > Source/WebCore/rendering/RenderBoxModelObject.cpp:285 > > + return thisBox->overridingContainingBlockContentLogicalHeight() == WTF::nullopt; > > I don't understand why this change affects only to replaced grid items. > Could you please elaborate on this ?
I think function hasAutoHeightOrContainingBlockWithAutoHeight() is only called by replaced handlers, right? I'm not sure why it's only for grid items though.
Javier Fernandez
Comment 6
2021-02-22 06:58:29 PST
Comment on
attachment 419408
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=419408&action=review
>>> Source/WebCore/rendering/RenderBoxModelObject.cpp:285 >>> + return thisBox->overridingContainingBlockContentLogicalHeight() == WTF::nullopt; >> >> I don't understand why this change affects only to replaced grid items. Could you please elaborate on this ? > > I think function hasAutoHeightOrContainingBlockWithAutoHeight() is only called by replaced handlers, right? I'm not sure why it's only for grid items though.
I missed that one of the clauses checks that this is indeed a grid item. However, I still have doubts about this change. I believe we indeed use in the GridTrackSizingAlgorithm logic a nullopt value for setting an indefinite size to the grid area (the grid item's override size). However, I've found some places where we still use -1 for that purpose (eg. RenderGrid::placeItemsOnGrid). I think nullopt is the correct choice, but I'd be more comfortable if we double check and correct the lines where we still use -1. I'd like to have layout tests to ensure the changes are verified. Another idea to consider is that if this is a grid item, it shouldn't be valid that hasOverridingContainingBlockContentLogicalHeight(), since we initialize of the values, if I'm not wrong. We could add change this conditional clause for an assert.
zsun
Comment 7
2021-02-23 08:05:15 PST
Created
attachment 421312
[details]
Patch
Sergio Villar Senin
Comment 8
2021-02-23 08:14:54 PST
Haven't checked the patch in detail but note that Rob is removing the usage of -1 in grid here
https://bugs.webkit.org/show_bug.cgi?id=221439
. We should use nullopt in those cases.
Javier Fernandez
Comment 9
2021-02-23 12:50:02 PST
(In reply to Sergio Villar Senin from
comment #8
)
> Haven't checked the patch in detail but note that Rob is removing the usage > of -1 in grid here
https://bugs.webkit.org/show_bug.cgi?id=221439
. We should > use nullopt in those cases.
Good to know. We should definitively wait for that patch to land before considering this. I don't have permissions for editing
bug 221439
, but probably it'd be a good idea to mark it as blocker for this.
zsun
Comment 10
2021-02-25 06:12:41 PST
Created
attachment 421519
[details]
Patch
zsun
Comment 11
2021-02-26 07:21:39 PST
Created
attachment 421642
[details]
Patch
Javier Fernandez
Comment 12
2021-02-26 09:23:20 PST
Comment on
attachment 421642
[details]
Patch r=me Lets wait for the EWS bots, though, but it looks good.
EWS
Comment 13
2021-03-08 13:12:59 PST
Committed
r274099
: <
https://commits.webkit.org/r274099
> All reviewed patches have been landed. Closing bug and clearing flags on
attachment 421642
[details]
.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug