Closed
Bug 1356916
Opened 7 years ago
Closed 7 years ago
stylo: inline canvas width and height are not reflected
Categories
(Core :: CSS Parsing and Computation, defect, P3)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: hiro, Assigned: hiro)
References
Details
Attachments
(3 files)
layout/reftests/canvas/size-change-1.html fails because this.
Assignee | ||
Comment 1•7 years ago
|
||
We need to call PostRestyleEvent in ServoRestyleManager::AttributeChanged [1], when we change canvas element's width and height.
[1] https://hg.mozilla.org/mozilla-central/file/17d8a1e278a9/layout/base/ServoRestyleManager.cpp#l504
Assignee | ||
Comment 2•7 years ago
|
||
(In reply to Hiroyuki Ikezoe (:hiro) from comment #1)
> We need to call PostRestyleEvent in ServoRestyleManager::AttributeChanged
> [1], when we change canvas element's width and height.
This is wrong. We need to call FrameNeedsReflow (or something) somehow instead of PostRestyleEvent().
Assignee | ||
Comment 3•7 years ago
|
||
dom/canvas/test/test_bitmaprenderer.html also depends on this.
Assignee | ||
Updated•7 years ago
|
Blocks: stylo-style-mochitest
Assignee | ||
Comment 4•7 years ago
|
||
(In reply to Hiroyuki Ikezoe (:hiro) from comment #2)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #1)
> > We need to call PostRestyleEvent in ServoRestyleManager::AttributeChanged
> > [1], when we change canvas element's width and height.
>
> This is wrong. We need to call FrameNeedsReflow (or something) somehow
> instead of PostRestyleEvent().
I was probably correct. In GeckoRestyleManager::AttributeChanged() we call PostRestyleEvent() with change hint obtained by Element::GetAttributeChangeHint.
Here is a try run with calling PostRestyleEvent() with the change hint.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=78bcfd813587cb0fe81f940aa90007b7a498f674
A dozen reftests passed.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 6•7 years ago
|
||
mozreview-review |
Comment on attachment 8865690 [details]
Bug 1356916 - Call PostRestyleEvent() with the change hint obtained by Element::GetAttributeChangeHint in ServoRestyleManager::AttributeChanged.
https://reviewboard.mozilla.org/r/137320/#review140322
::: layout/base/ServoRestyleManager.cpp:568
(Diff revision 1)
> + nsChangeHint hint = aElement->GetAttributeChangeHint(aAttribute, aModType);
> + if (hint) {
> + PostRestyleEvent(aElement, eRestyle_StyleAttribute, hint);
> + }
This patch causes scrollbar-thumb atom leak, but if we do call FrameNeedsReflow() directly instead of PostRestyleEvent(), the leak does not happen. Also, if we do call ProcessRestyledFrames() instead of PostRestyleEvent(), the leak happens.
Assignee | ||
Comment 7•7 years ago
|
||
Comment on attachment 8865690 [details]
Bug 1356916 - Call PostRestyleEvent() with the change hint obtained by Element::GetAttributeChangeHint in ServoRestyleManager::AttributeChanged.
Oops. I did forgot to do hg qref -e to remove review request.
Attachment #8865690 -
Flags: review?(cam)
Assignee | ||
Comment 8•7 years ago
|
||
A clue for the leak is StyleChildRangeForReconstruct() call [1] in nsCSSFrameConstructor::ContentRangeInserted(). Without this call no leak happens.
[1] https://hg.mozilla.org/mozilla-central/file/b21b974d60d3/layout/base/nsCSSFrameConstructor.cpp#l8019
Comment 9•7 years ago
|
||
mozreview-review |
Comment on attachment 8865690 [details]
Bug 1356916 - Call PostRestyleEvent() with the change hint obtained by Element::GetAttributeChangeHint in ServoRestyleManager::AttributeChanged.
https://reviewboard.mozilla.org/r/137320/#review140500
::: layout/base/ServoRestyleManager.cpp:570
(Diff revision 1)
> primaryFrame->AttributeChanged(aNameSpaceID, aAttribute, aModType);
> }
>
> + nsChangeHint hint = aElement->GetAttributeChangeHint(aAttribute, aModType);
> + if (hint) {
> + PostRestyleEvent(aElement, eRestyle_StyleAttribute, hint);
Why eRestyle_StyleAttribute? I think it should be nsRestyleHint(0), because we don't want to update the style="" attribute rule (or do any other selector matching). We just want to store the nsChangeHint to be picked up later to be processed.
Assignee | ||
Comment 10•7 years ago
|
||
(In reply to Cameron McCormack (:heycam) from comment #9)
> Comment on attachment 8865690 [details]
> Bug 1356916 - Call PostRestyleEvent() with the change hint obtained by
> Element::GetAttributeChangeHint in ServoRestyleManager::AttributeChanged.
>
> https://reviewboard.mozilla.org/r/137320/#review140500
>
> ::: layout/base/ServoRestyleManager.cpp:570
> (Diff revision 1)
> > primaryFrame->AttributeChanged(aNameSpaceID, aAttribute, aModType);
> > }
> >
> > + nsChangeHint hint = aElement->GetAttributeChangeHint(aAttribute, aModType);
> > + if (hint) {
> > + PostRestyleEvent(aElement, eRestyle_StyleAttribute, hint);
>
> Why eRestyle_StyleAttribute? I think it should be nsRestyleHint(0), because
> we don't want to update the style="" attribute rule (or do any other
> selector matching).
Thank you Cameron! I did not think nsRestyleHint(0) is effective for, but yes, it worked.
> We just want to store the nsChangeHint to be picked up
> later to be processed.
I did not know this fact. That's really what I wanted. Thank you!
Assignee | ||
Comment 11•7 years ago
|
||
I am almost giving up chasing the leak cause in this bug. I've been running a leaked test (layout/forms/crashtests/166750-1.html) on gdb with watching ref count of URLExtraData for chrome://global/skin/scrollbars.css for two days, but not finished yet. I should find out another way to catch it.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 15•7 years ago
|
||
mozreview-review |
Comment on attachment 8865690 [details]
Bug 1356916 - Call PostRestyleEvent() with the change hint obtained by Element::GetAttributeChangeHint in ServoRestyleManager::AttributeChanged.
https://reviewboard.mozilla.org/r/137320/#review141468
Attachment #8865690 -
Flags: review?(cam) → review+
Comment 16•7 years ago
|
||
mozreview-review |
Comment on attachment 8866575 [details]
Bug 1356916 - Skip crash tests causing scrollbar-thumb atom leak.
https://reviewboard.mozilla.org/r/138180/#review141472
Attachment #8866575 -
Flags: review?(cam) → review+
Comment 17•7 years ago
|
||
mozreview-review |
Comment on attachment 8866576 [details]
Bug 1356916 - Update reftest expectations.
https://reviewboard.mozilla.org/r/138182/#review141476
Attachment #8866576 -
Flags: review?(cam) → review+
Assignee | ||
Comment 18•7 years ago
|
||
Thank you for the review!
Comment 19•7 years ago
|
||
Pushed by hikezoe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f21116c45c78
Call PostRestyleEvent() with the change hint obtained by Element::GetAttributeChangeHint in ServoRestyleManager::AttributeChanged. r=heycam
https://hg.mozilla.org/integration/autoland/rev/afb494173e2d
Skip crash tests causing scrollbar-thumb atom leak. r=heycam
https://hg.mozilla.org/integration/autoland/rev/aa71971b432b
Update reftest expectations. r=heycam
Comment 20•7 years ago
|
||
Pushed by hikezoe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3a137e3e4b7d
Additional reftest expectations fix. r=me
Assignee | ||
Comment 21•7 years ago
|
||
There were more passed tests that did not passed when I did a try on try server.
Comment 22•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/f21116c45c78
https://hg.mozilla.org/mozilla-central/rev/afb494173e2d
https://hg.mozilla.org/mozilla-central/rev/aa71971b432b
https://hg.mozilla.org/mozilla-central/rev/3a137e3e4b7d
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Updated•6 years ago
|
Assignee: nobody → hikezoe
You need to log in
before you can comment on or make changes to this bug.
Description
•