Closed Bug 1418433 Opened 7 years ago Closed 7 years ago

stylo: window.getComputedStyle(element, null) would not get correct data for display: none element

Categories

(Core :: CSS Parsing and Computation, defect, P2)

Product:
Core
Component:
CSS Parsing and Computation
Version:
58 Branch
Platform:
Unspecified
All
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox-esr52 --- unaffected
firefox57 - wontfix
firefox58 + fixed
firefox59 --- fixed

People

(Reporter: jagannathante, Assigned: chenpighead)

References

Details

(Keywords: regression)

Attachments

(7 files, 3 obsolete files)

Screenshot-2017-11-17 RobotShop Robot Store Robots Robot Parts Robot Kits Robot Toys.png
271.72 KB, image/png
Details
zipped semi-reduced html
218.94 KB, application/x-7z-compressed
Details
reduced html
586 bytes, text/html
Details
Another test case
473 bytes, text/html
Details
Bug 1418433 - add tests for style data update mechanism for non-displayed elements.
59 bytes, text/x-review-board-request
heycam
: review+
emilio
: review+
Details
Bug 1418433 - increment RestyleGeneration for undisplayed elements when invalidating servo stylist.
59 bytes, text/x-review-board-request
heycam
: review+
gchang
: approval-mozilla-beta+
Details
Bug 1418433 - move the implementation of SetStylistStyleSheetsDirty to .cpp file.
59 bytes, text/x-review-board-request
heycam
: review+
Details 
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:59.0) Gecko/20100101 Firefox/59.0
Build ID: 20171117100127

Steps to reproduce:

 Go to http://www.robotshop.com


Actual results:

Navigation menus are rendered incorrectly. They are placed one below the other. See screenshot.


Expected results:

The menu should render like a menubar.
[Tracking Requested - why for this release]: A layout regression from Firefox 57 probably due to Stylo. Confirming 56 is not affected.
Status: UNCONFIRMED → NEW
status-firefox57: --- → affected
status-firefox58: --- → affected
status-firefox59: --- → affected
status-firefox-esr52: --- → unaffected
tracking-firefox57: --- → ?
tracking-firefox58: --- → ?
Component: Untriaged → CSS Parsing and Computation
Ever confirmed: true
Keywords: regression, regressionwindow-wanted
Product: Firefox → Core
Yes, disabling Stylo fixes the page layout.
Priority: -- → P2
regression window:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=5f721a664bf64fed99184a866b60c24a6afcb3a0&tochange=62cebea1d1578461a423a9ca7848706455db9ea5
Blocks: stylo-nightly
OS: Unspecified → All
Summary: Top menus not rendered correctly for robotshop.com → stylo: Top menus not rendered correctly for robotshop.com
reproduced when browser width > approx. 1040px(1024+scrollbar16px)
FWIW, it can be seen in the inspector, the Navigation menu uses a HTML <ul> element, and there's a "mainTopMenuMobile" class is added unexpectedly. Once you change it to "mainTopMenuDesktop", the issue is gone. No idea what Stylo behavior could trigger this...
Attached file reduced html (obsolete) — 
This should display "true" if browser width is larger than 1000px.

But stylo on, it display "false"...
Attached file reducd html (obsolete) — 
It seems if STYLO is enabled,
window.getComputedStyle(element, null) would not return a live CSSStyleDeclaration object :(
Attachment #8930019 - Attachment is obsolete: true
Summary: stylo: Top menus not rendered correctly for robotshop.com → stylo: window.getComputedStyle(element, null) would not return a live CSSStyleDeclaration object
Attached file reduced html — 

        Attachment #8930022 -
        Attachment is obsolete: true

    
    

    
  

      
      
      
      

        Attached file
          
        Another test case
        — 
      

    


  

    
    

    
  
(In reply to Alice0775 White from comment #9)
> Created attachment 8930055 [details]
> Another test case

> It seems if STYLO is enabled,
> window.getComputedStyle(element, null) would not return a live
> CSSStyleDeclaration object :(

I think it indeed return a live CSSStyleDeclaration object, it's just not updated in time.

Try replace

```
alert(info.height == "10px");
```

with

```
setTimeout(function() { alert(info.height == "10px"); }, 5000);
```

then the testcase would return true.

I'd guess the update of CSSStyleDeclaration in Stylo is not that synchronous like Gecko. Hi Cameron, any idea what could happened? I can take a look at this, but might need some guidance.
Flags: needinfo?(cam)

    
    

    
  
uum,

However, just remove the following dummy call line, then it return true as expected.

 info.height; // dummy call


So, This is not only a timing issue but also CSSStyleDeclaration object caches something...

    
    

    
  
There must something wrong between the time we update <style> element and the time we get property from live CSSStyleDeclaration object. My guess would be the style data is not rebuilt correctly as expected. I tried to add a RebuildAllStyleData() call in nsComputedDOMStyle::UpdateCurrentStyleSources() [1] for Stylo and the issue is gone.

I'll keep investigating why Servo's style data is not set to dirty correctly in this case.


[1] https://searchfox.org/mozilla-central/rev/33c90c196bc405e628bc868a4f4ba29b992478c0/layout/style/nsComputedDOMStyle.cpp#969-974
Assignee: nobody → jeremychen
Flags: needinfo?(cam)

    
    

    
  
According to ServoStyleSet::RecordStyleSheetChange [1], we count on servo's invalidation mechanism to invalidate the elements/rules properly. After I dug a bit deeper, it seems that the invalidation analysis works correctly while collecting the data [2] (i.e., a <div> element is marked as an invalid_elements for testcase in comment 9). Then, while calling flush [3], we expect to have an exact match for the <div> element while invaliding the whole subtree in [4]. However, we always early return while processing the <div> element in [5] for not having a element data.

Actually, in this case, it seems all the ElementData of the leaf elements (i.e., the one that don't have any child element) are None. I've no idea if this is expected or not. If this is expected, then we might need to fix the existing invalidation algorithm. If not, then there might be something wrong in somewhere else.



[1] https://searchfox.org/mozilla-central/rev/a5d613086ab4d0578510aabe8653e58dc8d7e3e2/layout/style/ServoStyleSet.cpp#1029-1046
[2] https://searchfox.org/mozilla-central/rev/a5d613086ab4d0578510aabe8653e58dc8d7e3e2/servo/components/style/invalidation/stylesheets.rs#109
[3] https://searchfox.org/mozilla-central/rev/a5d613086ab4d0578510aabe8653e58dc8d7e3e2/servo/components/style/invalidation/stylesheets.rs#148
[4] https://searchfox.org/mozilla-central/rev/a5d613086ab4d0578510aabe8653e58dc8d7e3e2/servo/components/style/invalidation/stylesheets.rs#228
[5] https://searchfox.org/mozilla-central/rev/a5d613086ab4d0578510aabe8653e58dc8d7e3e2/servo/components/style/invalidation/stylesheets.rs#202

    
    

    
  
My guess would be the "display:none" setting in the <body> element that causes all the child elements of <body> have no ElementData. This might be for the restyling optimization, and if we insist to keep this optimization, we might need to handle the getComputedStyle separately.

    
    

    
  
Yes, it's a deliberate design decision to have descendants of display:none elements have no ElementData.

For computed style objects for undisplayed elements, we look at the "undisplayed restyle generation" in UpdateCurrentStyleSources, which is a counter that increments whenever any DOM mutation happens that might affect styles in the document.  If this value is different from the time when we first resolved a style context for the undisplayed element, then we throw away that cached style context (nsComputedDOMStyle::mStyleContext) and compute a new one.

Maybe we should increment this counter in ServoStyleSet::SetStylist(XBL)StyleSheetsDirty?

    
    

    
  
(In reply to Cameron McCormack (:heycam) from comment #15)
> Yes, it's a deliberate design decision to have descendants of display:none
> elements have no ElementData.
> 
> For computed style objects for undisplayed elements, we look at the
> "undisplayed restyle generation" in UpdateCurrentStyleSources, which is a
> counter that increments whenever any DOM mutation happens that might affect
> styles in the document.  If this value is different from the time when we
> first resolved a style context for the undisplayed element, then we throw
> away that cached style context (nsComputedDOMStyle::mStyleContext) and
> compute a new one.
> 
> Maybe we should increment this counter in
> ServoStyleSet::SetStylist(XBL)StyleSheetsDirty?

Aha, there it is!!!
In this case, then We should definitely increment the counter, but I'd prefer to do this in ServoStyleSet::UpdateStylist() since we may calling SetStylist(XBL)StyleSheetsDirty multiple times, and we only need to make sure the restyle generation for undisplayed element is increased once we really update the stylist.

    
    

    
      
  


  

    
    

    
      
  


  
Summary: stylo: window.getComputedStyle(element, null) would not return a live CSSStyleDeclaration object → stylo: window.getComputedStyle(element, null) would not get correct data for display: none element

    
    

    
  
Comment on attachment 8931942 [details]
Bug 1418433 - increment RestyleGeneration for undisplayed elements when invalidating servo stylist.

https://reviewboard.mozilla.org/r/203000/#review208318

::: layout/style/ServoStyleSet.cpp:1333
(Diff revision 1)
> +  if (mStylistState &
> +      (StylistState::StyleSheetsDirty | StylistState::XBLStyleSheetsDirty)) {

We don't need to do this check, since we assert at the top of the function that StylistNeedsUpdate().

        Attachment #8931942 -
        Flags: review?(cam) → review+

    
    

    
  
Comment on attachment 8931941 [details]
Bug 1418433 - add tests for style data update mechanism for non-displayed elements.

https://reviewboard.mozilla.org/r/202998/#review208320

::: layout/style/test/test_computed_style.html:13
(Diff revision 1)
>  </head>
>  <body>
>  <a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=">Mozilla Bug </a>
>  <p id="display"></p>
>  <div id="content" style="display: none">
> -  
> +  <div id="nonDisplayedDiv"></div>

I see that another sub-test in this file, test_bug_595650, adds a child element to #content with script, does its test, then removes the element.  Maybe we should do this same, to keep our new sub-test's effects isolated from the others.

        Attachment #8931941 -
        Flags: review?(cam) → review+

    
    

    
  
Once this lands, please nominate for uplifting to beta.

    
    

    
  
(In reply to Cameron McCormack (:heycam) from comment #20)
> Comment on attachment 8931942 [details]
> Bug 1418433 - increment RestyleGeneration for undisplayed elements when
> updating servo stylist.
> 
> https://reviewboard.mozilla.org/r/203000/#review208318
> 
> ::: layout/style/ServoStyleSet.cpp:1333
> (Diff revision 1)
> > +  if (mStylistState &
> > +      (StylistState::StyleSheetsDirty | StylistState::XBLStyleSheetsDirty)) {
> 
> We don't need to do this check, since we assert at the top of the function
> that StylistNeedsUpdate().

Ok, I'll remove this check.

(In reply to Cameron McCormack (:heycam) from comment #21)
> Comment on attachment 8931941 [details]
> Bug 1418433 - add a test for style data update mechanism for non-displayed
> elements.
> 
> https://reviewboard.mozilla.org/r/202998/#review208320
> 
> ::: layout/style/test/test_computed_style.html:13
> (Diff revision 1)
> >  </head>
> >  <body>
> >  <a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=">Mozilla Bug </a>
> >  <p id="display"></p>
> >  <div id="content" style="display: none">
> > -  
> > +  <div id="nonDisplayedDiv"></div>
> 
> I see that another sub-test in this file, test_bug_595650, adds a child
> element to #content with script, does its test, then removes the element. 
> Maybe we should do this same, to keep our new sub-test's effects isolated
> from the others.

Make sense. I'll fix this before landing. Thank you for the review.

(In reply to Cameron McCormack (:heycam) from comment #22)
> Once this lands, please nominate for uplifting to beta.

Will do.

    
    

    
      
  

    
    

    
      
  
Status: NEW → ASSIGNED

    
    

    
  
Pushed by jichen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/436723f33b10
add a test for style data update mechanism for non-displayed elements. r=heycam
https://hg.mozilla.org/integration/autoland/rev/761f84b8edb0
increment RestyleGeneration for undisplayed elements when updating servo stylist. r=heycam

    
    

    
  
Backed out in https://hg.mozilla.org/integration/autoland/rev/9fd9d940dcf0 for intermittent but frequent "why should we have flushed style again?" assertion failures like https://treeherder.mozilla.org/logviewer.html#?job_id=147752542&repo=autoland

    
    

    
      
  


  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

        Attachment #8932361 -
        Flags: review?(cam)

    
    

    
  
Comment on attachment 8932361 [details]
Bug 1418433 - move the implementation of SetStylistStyleSheetsDirty to .cpp file.

https://reviewboard.mozilla.org/r/203404/#review208846

        Attachment #8932361 -
        Flags: review?(cam) → review+

    
    

    
  
(In reply to Phil Ringnalda (:philor) from comment #27)
> Backed out in https://hg.mozilla.org/integration/autoland/rev/9fd9d940dcf0
> for intermittent but frequent "why should we have flushed style again?"
> assertion failures like
> https://treeherder.mozilla.org/logviewer.html#?job_id=147752542&repo=autoland

I've verified on my local that the intermittent has been fixed.

Let's see how it goes on try before landing:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2aa3fccd105783a883a274840d60645899f0c231

    
    

    
  
(In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #33)
> (In reply to Phil Ringnalda (:philor) from comment #27)
> > Backed out in https://hg.mozilla.org/integration/autoland/rev/9fd9d940dcf0
> > for intermittent but frequent "why should we have flushed style again?"
> > assertion failures like
> > https://treeherder.mozilla.org/logviewer.html#?job_id=147752542&repo=autoland
> 
> I've verified on my local that the intermittent has been fixed.
> 
> Let's see how it goes on try before landing:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=2aa3fccd105783a883a274840d60645899f0c231

Oops, looks like neither increase RestyleGeneration in ServoStyleSet::UpdateStylist() nor increase RestyleGeneration in SetStylistStyleSheetsDirty() are right.

The former may cause the disagreement of generation data and hit the assertion in nsComputedDOMStyle::UpdateCurrentStyleSources() [1]. The latter may overly increase the generation data even for the case that we decide to do a fully-invalidated restyle (see failures in comment 33).

Well, I think all we need to do is to make sure we fix this in a more accurate way, which means we should only increase the generation for non-fully-invalidated restyle case. I tried to call IncrementUndisplayedRestyleGeneration() in ServoStyleSet::RecordStyleSheetChange() and try looks fine then:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=470e21857929fab99e95e34094b5502080af7ad5



[1] https://searchfox.org/mozilla-central/rev/9f3bd430c2b132c86c46126a0548661de876799a/layout/style/nsComputedDOMStyle.cpp#1082-1086

    
    

    
  
(In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #34)
> (In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #33)
> > (In reply to Phil Ringnalda (:philor) from comment #27)
> > > Backed out in https://hg.mozilla.org/integration/autoland/rev/9fd9d940dcf0
> > > for intermittent but frequent "why should we have flushed style again?"
> > > assertion failures like
> > > https://treeherder.mozilla.org/logviewer.html#?job_id=147752542&repo=autoland
> > 
> > I've verified on my local that the intermittent has been fixed.
> > 
> > Let's see how it goes on try before landing:
> > https://treeherder.mozilla.org/#/
> > jobs?repo=try&revision=2aa3fccd105783a883a274840d60645899f0c231
> 
> Oops, looks like neither increase RestyleGeneration in
> ServoStyleSet::UpdateStylist() nor increase RestyleGeneration in
> SetStylistStyleSheetsDirty() are right.
> 
> The former may cause the disagreement of generation data and hit the
> assertion in nsComputedDOMStyle::UpdateCurrentStyleSources() [1]. The latter
> may overly increase the generation data even for the case that we decide to
> do a fully-invalidated restyle (see failures in comment 33).

I don't understand this, why is this a problem? Increasing the generation more than once should never be a problem.

> Well, I think all we need to do is to make sure we fix this in a more
> accurate way, which means we should only increase the generation for
> non-fully-invalidated restyle case. I tried to call
> IncrementUndisplayedRestyleGeneration() in
> ServoStyleSet::RecordStyleSheetChange() and try looks fine then:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=470e21857929fab99e95e34094b5502080af7ad5

I don't understand why would it be right to not increase the restyle generation when a rule is mutated from CSSOM...

    
    

    
  
(In reply to Emilio Cobos Álvarez [:emilio] from comment #35)
> (In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #34)
> > 
> > Oops, looks like neither increase RestyleGeneration in
> > ServoStyleSet::UpdateStylist() nor increase RestyleGeneration in
> > SetStylistStyleSheetsDirty() are right.
> > 
> > The former may cause the disagreement of generation data and hit the
> > assertion in nsComputedDOMStyle::UpdateCurrentStyleSources() [1]. The latter
> > may overly increase the generation data even for the case that we decide to
> > do a fully-invalidated restyle (see failures in comment 33).
> 
> I don't understand this, why is this a problem? Increasing the generation
> more than once should never be a problem.

I thought so, but increasing the generation (through SetStylistStyleSheetsDirty) indeed causes tab crashing for layout/style/test/test_media_queries_dynamic_xbl.html. Once I restore the SetStylistStyleSheetsDirty() call here [1] to its original version, the crash is gone. I guess we shouldn't increase the generation for the "rebuild all selector maps" case?

[1] https://searchfox.org/mozilla-central/rev/9f3bd430c2b132c86c46126a0548661de876799a/layout/style/ServoStyleSet.cpp#1022

> 
> > Well, I think all we need to do is to make sure we fix this in a more
> > accurate way, which means we should only increase the generation for
> > non-fully-invalidated restyle case. I tried to call
> > IncrementUndisplayedRestyleGeneration() in
> > ServoStyleSet::RecordStyleSheetChange() and try looks fine then:
> > https://treeherder.mozilla.org/#/
> > jobs?repo=try&revision=470e21857929fab99e95e34094b5502080af7ad5
> 
> I don't understand why would it be right to not increase the restyle
> generation when a rule is mutated from CSSOM...

For other cases in ServoStyleSet::RecordStyleSheetChange(), e.g., RuleChanged, we always rebuild the selector maps. I thought this includes undisplayed elements as well, no?

    
    

    
      
  

    
    

    
      
  

        Attachment #8932361 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 8931942 [details]
Bug 1418433 - increment RestyleGeneration for undisplayed elements when invalidating servo stylist.

This is different from the original r+ed patch, so might need another round of review.

Hi Emilio, since Cameron is not around at the moment, and you seems have a bit concern, could you feedback on this fix? Thanks.

        Attachment #8931942 -
        Flags: feedback?(emilio)

    
    

    
  
(In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #36)
> (In reply to Emilio Cobos Álvarez [:emilio] from comment #35)
> > (In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #34)
> > > 
> > > Oops, looks like neither increase RestyleGeneration in
> > > ServoStyleSet::UpdateStylist() nor increase RestyleGeneration in
> > > SetStylistStyleSheetsDirty() are right.
> > > 
> > > The former may cause the disagreement of generation data and hit the
> > > assertion in nsComputedDOMStyle::UpdateCurrentStyleSources() [1]. The latter
> > > may overly increase the generation data even for the case that we decide to
> > > do a fully-invalidated restyle (see failures in comment 33).
> > 
> > I don't understand this, why is this a problem? Increasing the generation
> > more than once should never be a problem.
> 
> I thought so, but increasing the generation (through
> SetStylistStyleSheetsDirty) indeed causes tab crashing for
> layout/style/test/test_media_queries_dynamic_xbl.html. Once I restore the
> SetStylistStyleSheetsDirty() call here [1] to its original version, the
> crash is gone. I guess we shouldn't increase the generation for the "rebuild
> all selector maps" case?

No, the only reason it crashes is because we also create stylists for XBL, and those don't keep a reference to the pres-context, so mPresContext is null, and you crash.

What needs to happen is something like:

  if (mPresContext) {
    // XBL sheets don't have a pres context, but invalidating the restyle generation
    // in that case is handled by SetXBLStyleSheetsDirty in the "master" stylist.
    mPresContext->RestyleManager()->...();
  }

    
    

    
  
Comment on attachment 8931942 [details]
Bug 1418433 - increment RestyleGeneration for undisplayed elements when invalidating servo stylist.

f- because this doesn't handle invalidating it when a rule is added/removed/changed via CSSOM.

See the above comment for what really needs to happen.

        Attachment #8931942 -
        Flags: feedback?(emilio) → feedback-

    
    

    
  
(In reply to Emilio Cobos Álvarez [:emilio] from comment #41)
> f- because this doesn't handle invalidating it when a rule is
> added/removed/changed via CSSOM.

A test for that would be super-nice btw :)

    
    

    
  
(In reply to Emilio Cobos Álvarez [:emilio] from comment #40)
> (In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #36)
> > I thought so, but increasing the generation (through
> > SetStylistStyleSheetsDirty) indeed causes tab crashing for
> > layout/style/test/test_media_queries_dynamic_xbl.html. Once I restore the
> > SetStylistStyleSheetsDirty() call here [1] to its original version, the
> > crash is gone. I guess we shouldn't increase the generation for the "rebuild
> > all selector maps" case?
> 
> No, the only reason it crashes is because we also create stylists for XBL,
> and those don't keep a reference to the pres-context, so mPresContext is
> null, and you crash.
> 
> What needs to happen is something like:
> 
>   if (mPresContext) {
>     // XBL sheets don't have a pres context, but invalidating the restyle
> generation
>     // in that case is handled by SetXBLStyleSheetsDirty in the "master"
> stylist.
>     mPresContext->RestyleManager()->...();
>   }

I see, thanks for the quick feedback!

(In reply to Emilio Cobos Álvarez [:emilio] from comment #42)
> (In reply to Emilio Cobos Álvarez [:emilio] from comment #41)
> > f- because this doesn't handle invalidating it when a rule is
> > added/removed/changed via CSSOM.
> 
> A test for that would be super-nice btw :)

Sounds good, I'll add one. :)

    
    

    
      
  


  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
Looks like reviewboard loses the its history.... Cameron, I might need your stamp again for this r+ed patch.... :(
Flags: needinfo?(cam)

    
    

    
  
Comment on attachment 8932802 [details]
Bug 1418433 - move the implementation of SetStylistStyleSheetsDirty to .cpp file.

https://reviewboard.mozilla.org/r/203856/#review209352

        Attachment #8932802 -
        Flags: review?(cam) → review+

    
    

    
  
Comment on attachment 8931941 [details]
Bug 1418433 - add tests for style data update mechanism for non-displayed elements.

https://reviewboard.mozilla.org/r/202998/#review209366

::: layout/style/test/test_computed_style.html:761
(Diff revision 5)
> +     "computed value of display none element (removed)");
> +
> +  // Test for rule change, i.e., added/changed/removed
> +  is(cs.width, "auto",
> +     "computed value of display none element (before testing)");
> +  d.style.width = "100px";

Hmm, nope, this needs to do something like `style.sheet.insertRule(style.sheet.cssRules.length, "#nonDisplayedDiv { height: something }")` to catch your previous change.

        Attachment #8931941 -
        Flags: review?(emilio)

    
    

    
  
Comment on attachment 8931941 [details]
Bug 1418433 - add tests for style data update mechanism for non-displayed elements.

https://reviewboard.mozilla.org/r/202998/#review209366

> Hmm, nope, this needs to do something like `style.sheet.insertRule(style.sheet.cssRules.length, "#nonDisplayedDiv { height: something }")` to catch your previous change.

Oh, I've never used insertRule/deleteRule API before. Lesson learned.
There seems no such API for changing a rule with a given cssRules index, not sure if a combination of delete-then-insert counts....

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  
Flags: needinfo?(cam)

    
    

    
  
Comment on attachment 8931941 [details]
Bug 1418433 - add tests for style data update mechanism for non-displayed elements.

https://reviewboard.mozilla.org/r/202998/#review209478

        Attachment #8931941 -
        Flags: review?(emilio) → review+

    
    

    
  
Pushed by jichen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/69d52155845a
add tests for style data update mechanism for non-displayed elements. r=emilio,heycam
https://hg.mozilla.org/integration/autoland/rev/1790eb0797c8
move the implementation of SetStylistStyleSheetsDirty to .cpp file. r=heycam
https://hg.mozilla.org/integration/autoland/rev/2917fb414cad
increment RestyleGeneration for undisplayed elements when invalidating servo stylist. r=heycam

    
    

    
  
Comment on attachment 8931942 [details]
Bug 1418433 - increment RestyleGeneration for undisplayed elements when invalidating servo stylist.

Approval Request Comment
[Feature/Bug causing the regression]: Stylo
[User impact if declined]: authors who try to use CSSOM api to get style data and do some interactions for their website may cause annoying results for users. See the attached screenshot and the descriptions in comment 0.
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: no, we've got automated tests covered
[List of other uplifts needed for the feature/fix]: no
[Is the change risky?]: not that I’m aware of
[Why is the change risky/not risky?]: this only changes the cached style data for display: none elements, so the risk should be low to the rendering result.
[String changes made/needed]:

        Attachment #8931942 -
        Flags: approval-mozilla-beta?

    
    

    
  
Comment on attachment 8931942 [details]
Bug 1418433 - increment RestyleGeneration for undisplayed elements when invalidating servo stylist.

Fix a stylo regression in 57. Beta58+.

        Attachment #8931942 -
        Flags: approval-mozilla-beta? → approval-mozilla-beta+

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: