Closed
Bug 1411765
Opened 7 years ago
Closed 7 years ago
Fix reflow status when size is exactly the available size and undo comment changes
Categories
(Core :: Layout, defect)
Core
Layout
Tracking
()
RESOLVED
FIXED
mozilla58
Tracking | Status | |
---|---|---|
firefox58 | --- | fixed |
People
(Reporter: neerja, Assigned: neerja)
References
Details
Attachments
(6 files)
(deleted),
text/x-review-board-request
|
MatsPalmgren_bugz
:
review+
|
Details |
(deleted),
image/jpeg
|
Details | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
text/x-review-board-request
|
MatsPalmgren_bugz
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
MatsPalmgren_bugz
:
review+
|
Details |
This is a follow up bug filed for adding fantasai's patch here:
https://public.etherpad-mozilla.org/p/patches-accepted
The detailed explanation for this can be found in fantasai's comment here:
Bug 447660 Comment 9.
Comment hidden (mozreview-request) |
Comment 2•7 years ago
|
||
mozreview-review |
Comment on attachment 8922095 [details]
Bug 1411765 - Part1:Convert an Incomplete reflow status to OverflowIncomplete also when our size is exactly the available size, not just less.
https://reviewboard.mozilla.org/r/193098/#review198374
r=mats with the nits fixed, assuming it passed regression tests on Try on all platforms
::: commit-message-a97fb:2
(Diff revision 1)
> +Bug 1411765 - Undo comment changes in changeset b9c248025d37 and add new fragmentation test by fantasai. r?mats
> +
The current commit message only says things that are irrelevant. The relevant part of this patch is that we change "<" to "<=". So please change it to something like:
"Bug 1411765 - Convert an Incomplete reflow status to OverflowIncomplete also when our size is exactly the available size, not just less. r=mats"
(The "add new fragmentation test" is irrelevant because when a code change is accompanied by a reftest then it's obvious that it's testing the new layout.
The "by fantasai" is irrelevant because the patch meta data already names the patch author.
The "Undo comment changes in changeset b9c248025d37" is irrelevant because it follows that this patch is addressing those XXX comments and it doesn't really matter which exact revision a code comment was added.)
::: layout/generic/nsBlockFrame.cpp
(Diff revision 1)
>
> if (aStatus->IsIncomplete() &&
> - aFinalSize.BSize(wm) < aReflowInput.AvailableBSize()) {
> - // We fit in the available space - change status to OVERFLOW_INCOMPLETE.
> - // XXXmats why didn't Reflow report OVERFLOW_INCOMPLETE in the first place?
> + aFinalSize.BSize(wm) <= aReflowInput.AvailableBSize()) {
> + // We ran out of height on this page but we're incomplete
> + // Set status to complete except for overflow
> - // XXXmats and why exclude the case when our size == AvailableBSize?
nit: please add a "." at the end of both lines above to make them proper sentences.
::: layout/reftests/w3c-css/submitted/break3/reftest.list:1
(Diff revision 1)
> +== moz-block-fragmentation-001.html moz-block-fragmentation-001-ref.html
The raw patch says: "\ No newline at end of file"
Please add it.
Attachment #8922095 -
Flags: review?(mats) → review+
Comment 3•7 years ago
|
||
The bug title implies this a comment+test change but there's a code change at line 7494. Please make sure that's intentional & OK. Thx!
Assignee | ||
Updated•7 years ago
|
Summary: Undo comment changes made while re-enabling float breaking in columns → Fix reflow status when size is exactly the available size and undo comment changes
Comment hidden (mozreview-request) |
Assignee | ||
Comment 5•7 years ago
|
||
(In reply to Mats Palmgren (:mats) from comment #2)
Thanks Mats!
I fixed everything you said and the try run has some failures with grid-fragmentation + some crashes so I will look at those next week.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c0d3f1273108&selectedJob=139728791
Assignee | ||
Comment 6•7 years ago
|
||
Assignee | ||
Comment 7•7 years ago
|
||
Hi Mats,
I don't really understand grid fragmentation but from what I see in grid-fragmentation-019.html (and most other failures look similar to this), the difference between this and its reference is a small border fragment that you can see here:
https://hg.mozilla.org/mozilla-central/raw-file/tip/layout/tools/reftest/reftest-analyzer.xhtml#logurl=https://queue.taskcluster.net/v1/task/SRAioVt_SQqYt-cpwhXlQw/runs/0/artifacts/public/logs/live_backing.log&only_show_unexpected=1
This border fragment can be removed from the *reference* grid-fragmentation-019-ref.html without affecting the validity of the test like so:
https://public.etherpad-mozilla.org/p/grid-fragmentation-019-ref.htm_border_frag_removed
This makes me think that this is an existing bug in grid fragmentation code and the tests were written assuming this behavior.
I checked this on Chrome and Safari and they seem to do something much different. I attached a screenshot of this.
What do you think?
Flags: needinfo?(mats)
Comment 8•7 years ago
|
||
(In reply to Neerja Pancholi[:neerja] from comment #7)
> Hi Mats,
> This border fragment can be removed from the *reference*
> grid-fragmentation-019-ref.html without affecting the validity of the test
> like so:
> https://public.etherpad-mozilla.org/p/grid-fragmentation-019-ref.
> htm_border_frag_removed
Etherpad isn't a good tool for communicating changes like this.
You should've just created a patch as usual.
> This makes me think that this is an existing bug in grid fragmentation code
> and the tests were written assuming this behavior.
The Grid fragmentation code is correct, but the grid-fragmentation-019 tests
are affected by this block layout bug, so the reference layout is wrong.
As you suggest, removing the black border fragment in the third column in
the reference is the right fix. Since I created the patch while investigating
this I might as well attach it here. Please merge this into your patch and
land it (assuming everything else was green).
> I checked this on Chrome and Safari and they seem to do something much
> different. I attached a screenshot of this.
The grid fragmentation code in Blink/WebKit is very much *not* implementing
what the Grid spec says, so their layout is almost always wrong.
We do implement grid fragmentation per spec.
Flags: needinfo?(mats)
Assignee | ||
Comment 9•7 years ago
|
||
(In reply to Mats Palmgren (:mats) from comment #8)
Thanks Mats! I will keep what you mentioned in mind.
Other than this extra border fragment problem, there are two other problems of note with the try build:
1. Assertion failure -
layout/generic/crashtests/399412-1.html | assertion count 1 is more than expected 0 assertions
ASSERTION: overflow containers must be zero-block-size: 'finalSize.BSize(wm) == 0', file /Users/npancholi/builds/mozilla-central/layout/generic/nsBlockFrame.cpp, line 1676
I tried debugging this one and it is really not obvious to me so I will create a secondary bug for this.
2. Reftest failure with continuation in the wrong place -
grid-fragmentation-dyn1-006.html
This can be seen here (2nd test in link)
https://hg.mozilla.org/mozilla-central/raw-file/tip/layout/tools/reftest/reftest-analyzer.xhtml#logurl=https://queue.taskcluster.net/v1/task/WQ7xUZ2MSbmPDHgk3TW6qQ/runs/0/artifacts/public/logs/live_backing.log&only_show_unexpected=1
This is not reproducible on my 10.13 machine with the latest Nightly release but consistently fails on try.
So I think the options here are to either remove the '<=' change for now and take this patch with just comment changes
or
I can mark these problems as expected/fuzzed for now + change the reference file above to remove the border fragment
and
In both cases, file a secondary bug to investigate the final solution.
As the reviewer, which one of these sounds most reasonable to you? Or neither?
Comment 10•7 years ago
|
||
(In reply to Neerja Pancholi[:neerja] from comment #9)
> 1. Assertion failure -
> layout/generic/crashtests/399412-1.html | assertion count 1 is more than
> expected 0 assertions
That assertion is already known to fail for some tests (bug 574889)
so I think it's OK to mark this as assertimg in the manifest with
a comment referencing that bug.
> 2. Reftest failure with continuation in the wrong place -
> grid-fragmentation-dyn1-006.html
Oh, I missed this one the last time. I can reproduce it locally
so I'll look into it...
Flags: needinfo?(mats)
Assignee | ||
Comment 11•7 years ago
|
||
(In reply to Mats Palmgren (:mats) from comment #10)
Thanks for taking a look at this Mats. :)
I will update the patch with these changes (minus the fix for #2) tomorrow.
Comment 12•7 years ago
|
||
Please merge this patch too. That test should now pass.
I'll deal with it in bug 1415301.
Flags: needinfo?(mats)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 16•7 years ago
|
||
(In reply to Mats Palmgren (:mats) from comment #12)
> Created attachment 8926076 [details] [diff] [review]
> disable-failing-grid-fragmentation-test
>
> Please merge this patch too. That test should now pass.
> I'll deal with it in bug 1415301.
Done.
Try run looks good. There are a few failures but none of them seem related.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9e69496ca64d1d781982572b2d6417bfcac0fdd8&selectedJob=142831867
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 23•7 years ago
|
||
mozreview-review |
Comment on attachment 8926185 [details]
Bug 1411765 - Part2:Remove extra border fragment from ref file to match it with new expected output of grid fragmentation tests and change assertion count for crashtests.
https://reviewboard.mozilla.org/r/197434/#review202664
Attachment #8926185 -
Flags: review?(mats) → review+
Comment 24•7 years ago
|
||
mozreview-review |
Comment on attachment 8926186 [details]
Bug 1411765 - Part3:Disable part of failing test grid-fragmentation-dyn1-006.html.
https://reviewboard.mozilla.org/r/197436/#review202666
Thanks!
Attachment #8926186 -
Flags: review?(mats) → review+
Comment 25•7 years ago
|
||
Pushed by npancholi@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0590cfeeeaa3
Part1:Convert an Incomplete reflow status to OverflowIncomplete also when our size is exactly the available size, not just less. r=mats
https://hg.mozilla.org/integration/autoland/rev/4288a7814f71
Part2:Remove extra border fragment from ref file to match it with new expected output of grid fragmentation tests and change assertion count for crashtests. r=mats
https://hg.mozilla.org/integration/autoland/rev/a22464e2572f
Part3:Disable part of failing test grid-fragmentation-dyn1-006.html. r=mats
Comment 26•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/0590cfeeeaa3
https://hg.mozilla.org/mozilla-central/rev/4288a7814f71
https://hg.mozilla.org/mozilla-central/rev/a22464e2572f
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox58:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in
before you can comment on or make changes to this bug.
Description
•