977757 - Implement enabling will-change for Gaia or otherwise privileged content at first

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement)

Description

[Benoit Jacob [:bjacob] (mostly away)]

I'm not taking part in the conversation of whether we want to do this, but it looks like it could be useful to have the implementation ready, should we want to do this for b2g 1.4, while remaining issues are resolved to allow enabling will-change for general Web content.

What seems nontrivial (so to speak) is that the CSS_PROP_DISPLAY macro doesn't seem to apply here, since it seems that it only allows to enable a property based on a preference's value, not based on principal. Opinions welcome on how to implement this... by default, I'm looking into how CSS_PROP_DISPLAY is implemented and making a custom implementation based on that, checking some condition on the principal (not sure yet what that condition should be).

Comment 1

[Benoit Jacob [:bjacob] (mostly away)]

Actually, this isn't trivial to implement at all. The places where it is determined whether a CSS property is enabled don't seem to have an obvious way to get at the principal, from a quick look...

Comment 2

[Boris Zbarsky [:bzbarsky]]

The CSS parser has an mSheetPrincipal member.

Comment 3

[Benoit Jacob [:bjacob] (mostly away)]

Thanks for the tip!

Comment 4

[Boris Zbarsky [:bzbarsky]]

Note that we already have ways to flag CSS props as only available in UA sheets, via the property flags.  We could add some bits for "chrome only" or "certified app only" or similar...

Comment 5

[Benoit Jacob [:bjacob] (mostly away)]

Thanks, that makes a lot of sense.

If I understand correctly:
 - edit nsCSSProps.h to add a CSS_PROPERTY_ALWAYS_ENABLED_IN_PRIVILEGED.
 - make will-change have this flag, in nsCSSPropList.h.
 - edit nsCSSParser::ParseSheet to check if we are parsing privileged content, set a flag on *this similar to the existing CSSParserImpl::mUnsafeRulesEnabled, maybe CSSParserImpl::mPrivilegedRulesEnabled.
 - make change nsCSSProps::IsEnabled take a bitfield rather than just an enum, i.e. make EnabledState a bitfield so we can enable a property for privileged content independently of enabling it for UA sheets.
 - make CSSParserImpl::ParseProperty pass the correct bitfield to nsCSSProps::IsEnabled, based on CSSParserImpl::mPrivilegedRulesEnabled.

There is one big piece (at least!) that I am still missing. How are we going to tell if we are in a Certified App? Is there a way to tell that from the aBaseURI or aSheetURI or aSheetPrincipal received by nsCSSParser::ParseSheet() ? I understand that I can go from a manifest URI to a mozIApplication to the certification status. What I don't know, I suppose, is how to go from the URI of a random style sheet to any of that.

Comment 6

[Boris Zbarsky [:bzbarsky]]

That sounds reasonable to me as an implementation plan.

You can tell whether a principal is for a certified app by testing principal->GetAppStatus() == nsIPrincipal::APP_STATUS_CERTIFIED.  I believe that should do the right thing here.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Note that we will also want to do something about hiding the property in the WebIDL and probably do a similar check in ParseProperty.

Comment 8

[Benoit Jacob [:bjacob] (mostly away)]

(In reply to Boris Zbarsky [:bz] (reviews will be slow; ask someone else) from comment #7)
> Note that we will also want to do something about hiding the property in the
> WebIDL

If you would like me to do that, please give me a hint as to how that could be done.

> and probably do a similar check in ParseProperty.

The patches I'm about to upload here do this in LookupEnabledProperty() which is where the existing check for eEnabledInUASheets was made.

Comment 9

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Part 1: make EnabledState a bitfield

EnabledState was previously a 3-state enum with values "Enabled", "EnabledInUASheets" and "Any", with "Enabled" apparently meaning "only allow this property if it is globally enabled" and "Any" meaning "allow this property unconditionally".

This patch makes this a bit-field to make room for a new degree of freedom that will be added by the next patch: being enabled in privileged content only. This patch also renames "Any" to the hopefully more explicit "EnabledAlways", and removes the old "Enabled" value as, being the default value for this bitfield, it is tautologically zero, and "Enabled" sounded confusing. Some functions that take a EnabledState parameter now have it default to zero, so that many callers that were passing Enabled don't need to specify it anymore.

Comment 10

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Part 2: add EnabledInPrivilegedContent bit, and use it for will-change

This patch contains some code and behavior blindly copied from EnabledInUASheets without really understanding it, such as the part about not applying to aliases.

Comment 11

[Benoit Jacob [:bjacob] (mostly away)]

Note: I've manually tested that this correctly enables will-change in B2G certified apps and in desktop Firefox chrome.

Comment 12

[Boris Zbarsky [:bzbarsky]]

>+    dom::IsChromeURI(aSheetURI) ||

I guess we need that because chrome:// sheets mostly don't have the system principal?

> If you would like me to do that, please give me a hint as to how that could be done.

Looking at your patch, we will need three pieces, I think:

1) A function that returns true if either dom::IsInCertifiedApp or nsContentUtils::ThreadsafeIsCallerChrome() is true (we don't need to worry about non-system chrome:// stuff, since we're talking about scripts here).

2) Changes to dom/webidl/CSS2PropertiesProps.h to pass through the flags that include CSS_PROPERTY_ALWAYS_ENABLED_IN_PRIVILEGED_CONTENT so the python script that outputs the IDL in this case can see them.

3) Changes to GenerateCSS2PropertiesWebIDL.py to add Func=YourFunctionName to the extended attributes in cases when the flags contain CSS_PROPERTY_ALWAYS_ENABLED_IN_PRIVILEGED_CONTENT

One other question.  How does getPropertyValue("will-change") on computed style behave with these patches?

Comment 13

[Benoit Jacob [:bjacob] (mostly away)]

(In reply to Boris Zbarsky [:bz] (reviews will be slow; ask someone else) from comment #12)
> >+    dom::IsChromeURI(aSheetURI) ||
> 
> I guess we need that because chrome:// sheets mostly don't have the system
> principal?

I've just tried removing this dom::IsChromeURI(aSheetURI) condition, and without it, aboutTelemetry.css is no longer considered privileged, i.e. just the

    aSheetPrincipal->GetAppStatus() == nsIPrincipal::APP_STATUS_CERTIFIED

does not return true for desktop Firefox chrome sheets.

Should I try a different condition instead of IsChromeURI(aSheetURI)?

Comment 14

[Benoit Jacob [:bjacob] (mostly away)]

(In reply to Boris Zbarsky [:bz] (reviews will be slow; ask someone else) from comment #12)
> > If you would like me to do that, please give me a hint as to how that could be done.
> 
> Looking at your patch, we will need three pieces, I think:
> 
> 1) A function that returns true if either dom::IsInCertifiedApp or
> nsContentUtils::ThreadsafeIsCallerChrome() is true (we don't need to worry
> about non-system chrome:// stuff, since we're talking about scripts here).
> 
> 2) Changes to dom/webidl/CSS2PropertiesProps.h to pass through the flags
> that include CSS_PROPERTY_ALWAYS_ENABLED_IN_PRIVILEGED_CONTENT so the python
> script that outputs the IDL in this case can see them.
> 
> 3) Changes to GenerateCSS2PropertiesWebIDL.py to add Func=YourFunctionName
> to the extended attributes in cases when the flags contain
> CSS_PROPERTY_ALWAYS_ENABLED_IN_PRIVILEGED_CONTENT

OK, I see, thanks for the explanation. One question: this is a very different way of determining privileges than what I implemented in my above patches, because this seems to depend on properties of the current JS context; by contrast, my patches here were determining this based on the origin of the sheet, which seemed natural to do, at least there as these places of the layout/style code were already making checks based on sheet origins. If I implement the conditional-hiding-in-WebIDL based on these JS-context-based checks as you suggests, then it seems like we'll have two quite different types of checks done in these two different places, and they could (at least conceptually) sometimes not agree. How much should I worry about that?

I don't suppose that it would be possible (or even make sense) to have the conditional-hiding-in-WebIDL implemented purely in terms of an origin check, e.g. checking the origin of the script element (I suppose) ?

> 
> One other question.  How does getPropertyValue("will-change") on computed
> style behave with these patches?

I don't know enough about layout/style to even write a test page that would answer this question :-)

Comment 15

[Boris Zbarsky [:bzbarsky]]

> Should I try a different condition instead of IsChromeURI(aSheetURI)?

No, I think IsChromeURI is the best we can do at the moment given how our chrome sheets work.

> How much should I worry about that?

I don't think you should.  The primary purpose of the webidl bits is to make element.style.willChange not exist when the script poking at element.style is not in chrome or a privileged app.

> to have the conditional-hiding-in-WebIDL implemented purely in terms of an origin check

That is in fact how it would work with my suggestions, no?  It would check the principal of the script that's currently running and check whether that principal is system or is a certified app.

> to even write a test page that would answer this question

  <script>
    alert(getComputedStyle(document.documentElement).getPropertyValue("will-change"));
  </script>

If that shows "auto" even with the patches in this bug, we should fix it, probably by doing a security check in nsComputedDOMStyle::DoGetWillChange and if it fails returning null.

Though also, there's things like getComputedStyle(document.documentElement).length and whether enumerating the indexed properties on getComputedStyle(document.documentElement) ever returns "will-change".  That code is really not set up to report different sets of properties to different callers right now.  :(

Comment 16

[Boris Zbarsky [:bzbarsky]]

Maybe we're ok with just making will-change effectively readonly via computed style, though...

Comment 17

[Benoit Jacob [:bjacob] (mostly away)]

(In reply to Boris Zbarsky [:bz] (reviews will be slow; ask someone else) from comment #15)
> > Should I try a different condition instead of IsChromeURI(aSheetURI)?
> 
> No, I think IsChromeURI is the best we can do at the moment given how our
> chrome sheets work.
> 
> > How much should I worry about that?
> 
> I don't think you should.  The primary purpose of the webidl bits is to make
> element.style.willChange not exist when the script poking at element.style
> is not in chrome or a privileged app.
> 
> > to have the conditional-hiding-in-WebIDL implemented purely in terms of an origin check
> 
> That is in fact how it would work with my suggestions, no?

I see, that's what I didn't understand above, thanks.

>   <script>
>    
> alert(getComputedStyle(document.documentElement).getPropertyValue("will-
> change"));
>   </script>
> 
> If that shows "auto" even with the patches in this bug, we should fix it,
> probably by doing a security check in nsComputedDOMStyle::DoGetWillChange
> and if it fails returning null.

Thanks. This currently displays nothing in the default case where will-change is disabled, and displays "auto" when will-change is enabled. So I suppose I don't need to do anything more on this front.

I'll look at implementing your WebIDL suggestion now...

Comment 18

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Part 3: expose CSS properties on JS objects as needed

Comment 19

[Benoit Jacob [:bjacob] (mostly away)]

Asking you to review these patches because you've already started, and this is really time-sensitive (*); feel free to reorient me to other reviewers.

(*) : the hope here is we can have will-change available to Gaia in Firefox OS 1.4. To that effect, it would be really nice to have this by the end of this week; and I am leaving for a week-long vacation at the end of this week, too.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8385761 [details] [diff] [review]
Part 3: expose CSS properties on JS objects as needed

Putting a Func on each one of these, with different pointers, is actually fairly bad; it will noticeably slow down creation of the prototype object due to all the function calls.  We should really only output the annotation on things that need to be conditional.

And I'd like dbaron to OK the fact that computed style will leak the fact that we actually support this property....

Comment 21

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8384684 [details] [diff] [review]
Part 1: make EnabledState a bitfield

The EnabledState argument didn't have a default value for a reason: the idea is to make people actually think about what they're asking for.  Please don't change that.

Comment 22

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8384687 [details] [diff] [review]
Part 2: add EnabledInPrivilegedContent bit, and use it for will-change

>+    if ((aEnabled & eEnabledInPrivilegedContent) &&
>+       PropHasFlags(aProperty, CSS_PROPERTY_ALWAYS_ENABLED_IN_PRIVILEGED_CONTENT))

The indent here is weird.

r=me, but I'm a bit worried about the naming.  We have "privileged apps" on b2g, but this flag does NOT enable properties in those.

It might be better to just call this flag CSS_PROPERTY_ALWAYS_ENABLED_IN_CHROME_OR_CERTIFIED_APP and the bit eEnabledInChromeOrCertifiedApp or something.

Comment 23

[Benoit Jacob [:bjacob] (mostly away)]

(In reply to Boris Zbarsky [:bz] (reviews will be slow; ask someone else) from comment #20)
> Comment on attachment 8385761 [details] [diff] [review]
> Part 3: expose CSS properties on JS objects as needed
> 
> Putting a Func on each one of these, with different pointers, is actually
> fairly bad; it will noticeably slow down creation of the prototype object
> due to all the function calls.  We should really only output the annotation
> on things that need to be conditional.

Regarding the different function pointers, note that the template here is a MOZ_ALWAYS_INLINE wrapper around a non-templated function, as an attempt to solve that problem.

I tried doing what you suggest, but the python script doesn't know about the 1<<23 value of the relevant CSS_PROPERTY_ALWAYS_ENABLED_IN_PRIVILEGED_CONTENT flag here. I could get by by stringifying the flags and looking for the substring "CSS_PROPERTY_ALWAYS_ENABLED_IN_PRIVILEGED_CONTENT". Or I could hardcode 1<<23 in python. Is either way acceptable, or do you see an alternative?

Oh, here is an alternative: we could further templatize IsCSSPropertyExposedToJS on the flags, so as to evaporate at compile time the branch here. How about that?

Comment 24

[Boris Zbarsky [:bzbarsky]]

> as an attempt to solve that problem.

The bindings code in this case has a table with actual function pointers in it, not just code that calls the templated function.  Furthermore, it tries to coalesce identical function invocations, but in this case they all look different to the bindings code, so it can't coalesce them.

> I could get by by stringifying the flags and looking for the substring
> "CSS_PROPERTY_ALWAYS_ENABLED_IN_PRIVILEGED_CONTENT".

That seems perfectly reasonable to me.

Comment 25

[Benoit Jacob [:bjacob] (mostly away)]

(In reply to Boris Zbarsky [:bz] (reviews will be slow; ask someone else) from comment #24)
> > as an attempt to solve that problem.
> 
> The bindings code in this case has a table with actual function pointers in
> it, not just code that calls the templated function.  Furthermore, it tries
> to coalesce identical function invocations, but in this case they all look
> different to the bindings code, so it can't coalesce them.

Ah, I see.

An alternative (to having to templatize this function) would be to be able to pass it some "user data" i.e. have the func take more than just a JSContext* and a JSObject*. Although I'm not sure what form that would take here, as the side writing that user data would be python.

Comment 26

[Boris Zbarsky [:bzbarsky]]

There is no problem with templatizing the function.  I just don't want us to put it on every single accessor on CSS2Properties.  Putting it on only the ones that have CSS_PROPERTY_ALWAYS_ENABLED_IN_PRIVILEGED_CONTENT should be fine.

Comment 27

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Part 3: expose CSS properties on JS objects as needed

This stringifies the flags and has python look for the ENABLED_IN_PRIVILEGED flag as a substring, to determine whether to generate a Func= annotation.

Comment 28

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8386241 [details] [diff] [review]
Part 3: expose CSS properties on JS objects as needed

>+    elif pref is not "":

Please document that this is an elif because the flag overrides the pref, and that it's therefore the responsibility of IsCSSPropertyExposedToJS<> to check the pref as needed?

r=me

Comment 29

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Part 1: make EnabledState a bitfield

Applied Boris' above review comments + followup IRC conversation on #content today.

Comment 30

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Part 2: add EnabledInChromeOrCertifiedApp bit, and use it for will-change (carries r=bz)

Applied Boris' review comments.

Comment 31

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Part 3: expose CSS properties on JS objects as needed (carries r=bz)

Applied Boris' review comments. Previously r+'d.

Comment 32

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Part 2: add EnabledInChromeOrCertifiedApp bit, and use it for will-change (carries r=bz)

Renamed mPrivilegedRulesEnabled to adjust to other renaming, and added a comment.

Comment 33

[Benoit Jacob [:bjacob] (mostly away)]

https://tbpl.mozilla.org/?tree=Try&rev=704047c39194

Comment 34

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Comment on attachment 8386398 [details] [diff] [review]
Part 1: make EnabledState a bitfield

>-  // Given a property string, return the enum value
>-  enum EnabledState {
>-    eEnabled,
>-    eEnabledInUASheets,
>-    eAny
>-  };
>+  typedef uint8_t EnabledState;
>+  // The default EnabledState: only enable what's enabled for all content,
>+  // given the current values of preferences.
>+  static const EnabledState eEnabledForAllContent = 0;
>+  // Special value to unconditionally enable a property. This implies all the
>+  // bits below, but is strictly more than just their OR-ed union.
>+  // This just skips any test so a property will be enabled even if it would
>+  // have been disabled with all the bits below set.
>+  static const EnabledState eIgnoreEnabledState   = 0xff;
>+  // Enable a property in UA sheets.
>+  static const EnabledState eEnabledInUASheets    = 0x01;

Please keep this as an enum - you can put the bitfield values inside.

(And it seems like the eIgnoreEnabledState could just be 0x03, and then
0x07 after the second patch.  Or just 0x07.)

>-  // We intentionally don't support eEnabledInUASheets for aliases yet
>-  // because it's unlikely there will be a need for it.
>+  // We intentionally don't support eEnabledInUASheets
>+  // for aliases yet because it's unlikely there will be a need for it.

Leave this wrapped as it was.

Also, the changes in nsDOMWindowUtils.cpp, inDOMUtils.cpp, Declaration.cpp,
nsDOMCSSDeclaration.cpp, and the first change in nsRuleNode.cpp, and the
first and last change in TestCSSPropertyLookup.cpp run over 80 characters.

In nsCSSParser.cpp, it seems a little better to use a construct that
continues to use the eEnabledForAllContent constant explicitly rather
than writing 0.

r=dbaron with that fixed

Comment 35

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

(In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #34)
> In nsCSSParser.cpp, it seems a little better to use a construct that
> continues to use the eEnabledForAllContent constant explicitly rather
> than writing 0.

Actually, probably the right thing to do is static_assert that the constant is equal to 0, given that you're depending on that.

Comment 36

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8386398 [details] [diff] [review]
Part 1: make EnabledState a bitfield

r=me modulo dbaron's nits

Comment 37

[Benoit Jacob [:bjacob] (mostly away)]

(In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #34)
> Please keep this as an enum - you can put the bitfield values inside.

I didn't make this an enum because bitwise operators such as | between enums return an int, which can't be assigned back to that enum type; and in particular, |= and &= between enums just fails to compile. (Confirmed with with clang 3.3).

I tried really hard to make this enum and learn along the way how to make enum bitfields work. I found this, http://stackoverflow.com/questions/1448396/how-to-use-enums-as-flags-in-c , which has a great solution to the operator| problem: just define an overloaded operator| for the enum type.

But that doesn't solve the operator|= problem, which can't be overloaded for non-class types (since assignment ops need to be members).

Not finding a reasonable way to make enum bitfields work, and not supposing that you would like lots of casts, I kept the uint8_t!

I would definitely like stronger typing here than just uint8_t;  note though that c++98 enums are only slightly more typing than that, and don't give us all the typing that we would want. MOZ_BEGIN_ENUM_CLASS enums don't support operators for all I know, so that's not a solution either.

> 
> (And it seems like the eIgnoreEnabledState could just be 0x03, and then
> 0x07 after the second patch.  Or just 0x07.)

I actually much prefer 0xff, because it won't need to be updated everytime one adds a bit, and it conveys the intent to make this a special value, different from others.

***

Applied all other review comments.

Comment 38

[Cameron McCormack (:heycam)]

(In reply to Benoit Jacob [:bjacob] from comment #37)
> I didn't make this an enum because bitwise operators such as | between enums
> return an int, which can't be assigned back to that enum type; and in
> particular, |= and &= between enums just fails to compile. (Confirmed with
> with clang 3.3).
> 
> I tried really hard to make this enum and learn along the way how to make
> enum bitfields work. I found this,
> http://stackoverflow.com/questions/1448396/how-to-use-enums-as-flags-in-c ,
> which has a great solution to the operator| problem: just define an
> overloaded operator| for the enum type.
> 
> But that doesn't solve the operator|= problem, which can't be overloaded for
> non-class types (since assignment ops need to be members).

I don't think that's true:

  https://hg.mozilla.org/mozilla-central/file/8122ffa9e1aa/layout/generic/nsFrameState.h#l47

Comment 39

[Benoit Jacob [:bjacob] (mostly away)]

https://tbpl.mozilla.org/?tree=Try&rev=f2a4f817167d

Comment 40

[Benoit Jacob [:bjacob] (mostly away)]

(In reply to Cameron McCormack (:heycam) from comment #38)
> I don't think that's true:
> 
>  
> https://hg.mozilla.org/mozilla-central/file/8122ffa9e1aa/layout/generic/
> nsFrameState.h#l47

Thanks! I didn't realize that it was possible, in this case, to define assignment operators as global functions. Though this example is dealing with c++11 enum classes. I'll try to see if I can get the same with plain c++98 enums. Separately, it would be worth turning this into a c++11 enum class, using mfbt/TypedEnum.h.

Comment 41

[Benoit Jacob [:bjacob] (mostly away)]

Works like a charm locally with clang 3.3 / linux; try push to check that this builds everywhere:

https://tbpl.mozilla.org/?tree=Try&rev=8cd3938be157

Comment 42

[Benoit Jacob [:bjacob] (mostly away)]

Er, one minor compile fix in the 3rd patch... https://tbpl.mozilla.org/?tree=Try&rev=fe760f645f5e

Comment 43

[Benoit Jacob [:bjacob] (mostly away)]

another little oops... dont worry, I do cancel my bad try jobs each time... https://tbpl.mozilla.org/?tree=Try&rev=4e50dec612f9

Comment 44

[Benoit Jacob [:bjacob] (mostly away)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/73c710ddf3d8
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/9456495d6121
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/d65a803d2b9a

Comment 45

[Ryan VanderMeulen [:RyanVM]]

Backed out for Gaia UI test failures.
https://hg.mozilla.org/integration/mozilla-inbound/rev/afc3d7790123

https://tbpl.mozilla.org/php/getParsedLog.php?id=35730309&tree=Mozilla-Inbound

Comment 46

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Seems like Gaia developers have been adding CSS to their code when that CSS doesn't do anything yet:
  http://mxr.mozilla.org/gaia/search?string=will-change
which generally isn't a great idea.

(I'd think the opacity and transform ones are probably more suspicious since those lead to creation of a stacking context, though it could certainly be a bug elsewhere.)

Comment 47

[Benoit Jacob [:bjacob] (mostly away)]

Tells you how much they want it :-)

(And it did something, once you enabled the pref).

Comment 48

[Benoit Girard (:BenWa)]

We did that so we could begin test it. I didn't expect it to cause a regression since it's an optimization hint. This is certainly worth digging into.

Comment 49

[Boris Zbarsky [:bzbarsky]]

It's not just an optimization hint, as comment 46 points out.  It changes things like z-ordering and whatnot.

Comment 50

[Benoit Jacob [:bjacob] (mostly away)]

Running gaia ui tests locally seems to be too difficult for me --- after 1 hour of trying and getting help from #ateam. One trick that isn't mentioned on MDN is that the b2g desktop mozconfig must have ENABLE_MARIONETTE=1 in order to allow gaia ui tests to run, but that is not enough apparently. I think I'll use tryserver with a long iteration time...

Comment 51

[Zac C (:zac)]

This is the HTML report from the TBPL run linked to in comment #45 

http://mozilla-releng-blobs.s3.amazonaws.com/blobs/mozilla-inbound/sha512/2f100ca52272f49c0b9b4f7b120e67bd15d347b0f38d99826abfa7a8f6206ba31918bf082bd3e25be90af658bc1509227ac367411cbeb0893f87f846d4ef1170

You can see the context menu has not loaded fully, to the top of the status bar. You can replicate this manully by just loading desktopb2g, opening the UI tests app, finding the Context menu option and then loading the context menu.

So I think the patch has affected how context menu is rendered; running the tests won't really do much good for now.

Comment 52

[Benoit Girard (:BenWa)]

In the meantime mchang do you know if the system app will-change are used by the context menu?

http://mxr.mozilla.org/gaia/search?string=will-change&find=%2Fapps%2Fsystem%2Fstyle%2Fwindow.css&findi=&filter=^[^\0]*%24&hitlimit=&tree=gaia

Comment 53

[Mason Chang [Inactive] [:mchang]]

Hmm, I'm not sure. I didn't add those will-changes. It landed in bug 962649. Etienne, do you know from comment 52?

Comment 54

[Benoit Jacob [:bjacob] (mostly away)]

The funny thing is that before my patches here, everything works fine, *even* with the will-change pref set. In other words, the problem seems to be in my patches here, not in will-change behavior...

The problem is super easy to reproduce, as Zac suggested in comment 51, in fact, it causes most of the homescreen to be solid black.

Comment 55

[Benoit Jacob [:bjacob] (mostly away)]

Ah no, I was just running into artifacts of GL layers in the b2g desktop client on Linux...

Comment 56

[Benoit Jacob [:bjacob] (mostly away)]

OK, now I've followed Zac's exact steps to reproduce,

(In reply to Zac C (:zac) from comment #51)
> You can see the context menu has not loaded fully, to the top of the status
> bar. You can replicate this manully by just loading desktopb2g, opening the
> UI tests app, finding the Context menu option and then loading the context
> menu.

and indeed this reproduces very well, already before the present patches, with the will-change pref set.

Comment 57

[Benoit Jacob [:bjacob] (mostly away)]

The problem is specifically with will-change:transform. Indeed, if I disable the handling of 'transform' in nsRuleNode::ComputeDisplayData's handling of will-change, then the issue goes away.

Comment 58

[Benoit Jacob [:bjacob] (mostly away)]

Try push disabling will-change:transform handling, keeping other will-change support:

    https://tbpl.mozilla.org/?tree=Try&rev=702080abefd4

will-change:transform is 6 rules throughout Gaia at the moment, our of 28 will-change rules, so there would be value already in enabling will-change without transform while will-change:transform support matures...

Comment 59

[Benoit Jacob [:bjacob] (mostly away)]

Matt, do you have any guess as to why will-change:transform can results in a mis-transformed layer?

These are the places that are affected by will-change:transform:
http://dxr.mozilla.org/mozilla-central/search?q=%2Bmacro-ref%3ANS_STYLE_WILL_CHANGE_TRANSFORM

Comment 60

[Matt Woodrow (:mattwoodrow)]

At a guess, I'd say it forces a transform to be an active layer where previously it was an inactive one. And that's exposing a clipping bug on the compositor.

Comment 61

[Etienne Segonzac (:etienne)]

(In reply to Mason Chang [:mchang] from comment #53)
> Hmm, I'm not sure. I didn't add those will-changes. It landed in bug 962649.
> Etienne, do you know from comment 52?

It's for the app-to-app edge gesture, but I agree we probably got carried away.
We did test it on builds with the pref on, and designed it to only be enabled when the edge gesture setting is on (it's off by default).

But apparently this one [1] fell through and it's on the app window (which contains the context menu).

Feel free to remove it (or ask me to remove it) if it can help here.

[1] https://github.com/mozilla-b2g/gaia/blob/master/apps/system/style/window.css#L447

Comment 62

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: bug977757.test.failure.workaround.patch

The rendering error seems related to the handling of |position: fixed| element.

The context menu inherit |position: fixed| rule from https://github.com/mozilla-b2g/gaia/blob/master/shared/style/action_menu.css#L12

Then in the system app it is assumed that the context menu always starts at position 0, 0 and a |top: 2rem| is applied at https://github.com/mozilla-b2g/gaia/blob/master/apps/system/style/window.css#L387

Applying the patch above resolved the rendering issue for me, as the context menu will now be rendered relatively to its container.

So I'm not sure this is a clipping issue versus a miscalculation of the x,y origin of the element.

It will be nice if we can keep the |will-change: transform| support in the platform and in Gaia we can avoid to use will-change on |position: fixed| with |top: ..px| until the underlying bug is fixed in the platform. So I will be inclined to reland the platform patch couple with this little Gaia patch for this bug.

Comment 63

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

I'm also pretty sure that the |position: fixed; top: 2rem;| here is wrong since it means that context menu pop up in a fullscreen app will be mispositionned.

I sent a PR against Travis to see if this small css change will break some tests or not: https://travis-ci.org/mozilla-b2g/gaia/builds/20279281

Comment 64

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #63)
> I sent a PR against Travis to see if this small css change will break some
> tests or not: https://travis-ci.org/mozilla-b2g/gaia/builds/20279281

Travis is green.

Comment 65

[Benoit Jacob [:bjacob] (mostly away)]

Great, I could confirm locally that this 1-line gaia patch 'fixes' the bug. So we have two ways to land this now: either temporarily disable the handling of will-change:transform or land the gaia patch first.

Comment 66

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Benoit Jacob [:bjacob] from comment #65)
> Great, I could confirm locally that this 1-line gaia patch 'fixes' the bug.
> So we have two ways to land this now: either temporarily disable the
> handling of will-change:transform or land the gaia patch first.

IMO it's better to land the Gaia patch first as we can ensure that this bug can be workarounded in Gaia. The will-change: transform should help homescreen panning and that's a good thing.

Comment 67

[Etienne Segonzac (:etienne)]

Comment on attachment 8387527 [details] [diff] [review]
bug977757.test.failure.workaround.patch

Review of attachment 8387527 [details] [diff] [review]:
-----------------------------------------------------------------

That's a nice patch to land anyway :)

Comment 68

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Gaia patch landed on master: https://github.com/mozilla-b2g/gaia/commit/9ab5465b472186cefed6419a7a61614d1f84a41f

Comment 69

[Benoit Jacob [:bjacob] (mostly away)]

Your change is already reflected on integration/gaia-central:
http://hg.mozilla.org/integration/gaia-central/rev/b95e3f66303b

New try push with gaia.json edited to point to it:
https://tbpl.mozilla.org/?tree=Try&rev=b08deb7b026b

Comment 70

[Benoit Jacob [:bjacob] (mostly away)]

The Gu run is green! https://tbpl.mozilla.org/?tree=Try&rev=b08deb7b026b

And b2g-inbound has received the gaia.json update to pick up your change. So I should be able to land now.

Comment 71

[Benoit Jacob [:bjacob] (mostly away)]

remote:   https://hg.mozilla.org/integration/b2g-inbound/rev/75217485ce5a
remote:   https://hg.mozilla.org/integration/b2g-inbound/rev/fd02f1217e4c
remote:   https://hg.mozilla.org/integration/b2g-inbound/rev/45253e02bde3

Comment 72

[Benoit Jacob [:bjacob] (mostly away)]

Follow-up bug for will-change:transform on position:fixed: bug 981022.

Comment 73

[Benoit Jacob [:bjacob] (mostly away)]

We're sticking this time! \o/

Comment 74

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/75217485ce5a
https://hg.mozilla.org/mozilla-central/rev/fd02f1217e4c
https://hg.mozilla.org/mozilla-central/rev/45253e02bde3

Comment 75

[Peter Chang[:pchang]]

I found the landed patch cause bug 981804.

https://bugzilla.mozilla.org/show_bug.cgi?id=981804#c12

Comment 76

[Peter Chang[:pchang]]

Reopen this bug because of bug 981804, but the following comment suggests to check will-changes modification in gaia side.
https://bugzilla.mozilla.org/show_bug.cgi?id=981804#c16

Comment 77

[Jason Smith [:jsmith]]

Backout commit btw is here:

http://hg.mozilla.org/mozilla-central/rev/44ae8462d6ab

Comment 78

[Jason Smith [:jsmith]]

A workaround has been landed in Gaia to workaround the smoketest regression here, so we should be good to reland this.

Comment 79

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Relanded:
remote:   https://hg.mozilla.org/mozilla-central/rev/aaabd2d39060
remote:   https://hg.mozilla.org/mozilla-central/rev/71014b91b6c6
remote:   https://hg.mozilla.org/mozilla-central/rev/a10d0ba50f64