Don't call inIDOMUtils.getCSSStyleRules() for pseudo element (e.g. ::before) directly

RESOLVED FIXED in Firefox 58

Status

P3
normal
RESOLVED FIXED
a year ago
6 months ago

People

(Reporter: hiro, Assigned: hiro)

Tracking

unspecified
Firefox 58

Firefox Tracking Flags

(firefox57 fix-optional, firefox58 fixed)

Details

Attachments

(2 attachments)

(Assignee)

Description

a year ago
In devtools/server/css-logic.js, we call getBindingElementAndPseudo() to convert the pseudo element to a pair of the parent element and pseudo type, then call  inIDOMUtils.getCSSStyleRules().  But other call sites of the function don't call getBindingElementAndPseudo() at all.  We should do it just like this try;

https://hg.mozilla.org/try/rev/1a26929347eec33db87c6c6c4713b20eb8067255

Honestly I am surprised that the current code works fine without any problems (I guess we haven't yet noticed the problems).  The function might work to be called against pseudo element directly, but..
(Assignee)

Comment 1

a year ago
Note that I did notice this fact when I try to fix a stylo bug (bug 1398661) and add an assertion in a function that should not be called against pseudo element itself.  See 1398661 comment 7 and a try run which caused the assertion in browser_computed_pseudo-element_01.js.

https://treeherder.mozilla.org/logviewer.html#?job_id=130482024&repo=try&lineNumber=8179
Comment hidden (mozreview-request)
(Assignee)

Comment 3

a year ago
Patrick, would you mind reviewing this patch?  I am confident the patch does do the right thing to do, but I don't quit understand about handling external resources in devtools (i.e. lazyRequreGetter or require()), so I might be wrong how to use it.
Attachment #8907404 - Flags: review?(pbrosset) → review?(bgrinstead)
I think Brian knows the subtleties of using getBindingElementAndPseudo better than I do, so I've forwarded the review to him.

One thing I can see is that you are requiring a file in devtools/server from a file in devtools/client (indeed, in devtools/client/shared/test/test-actor.js you're requiring css-logic that's in the server folder).
When Firefox is packaged, this doesn't work. Only devtools/shared is being packaged in both locations.
Having said that, test-actor.js is only a test file, so I don't know if this rule applies here. If the tests pass, then it means it's probably ok to do this. We might want to ask Alexandre Poirot about this.
Flags: needinfo?(poirot.alex)
(In reply to Patrick Brosset <:pbro> from comment #4)
> I think Brian knows the subtleties of using getBindingElementAndPseudo
> better than I do, so I've forwarded the review to him.
> 
> One thing I can see is that you are requiring a file in devtools/server from
> a file in devtools/client (indeed, in
> devtools/client/shared/test/test-actor.js you're requiring css-logic that's
> in the server folder).

It may work, but you should not.

Could you move CssLogic.getBindingElementAndPseudo, from:
  http://searchfox.org/mozilla-central/source/devtools/server/css-logic.js#657-671
to: 
  http://searchfox.org/mozilla-central/source/devtools/shared/inspector/css-logic.js

And then pull it from devtools/shared/inspector/css-logic instead of devtools/server/css-logic.js?

If you don't want to change existing callsites currently using CssLogic.getBindingElementAndPseudo from devtools/server/css-logic.js, you can keep exposing it like this:
  const {getBindingElementAndPseudo} = require("devtools/shared/inspector/css-logic");
  CssLogic.getBindingElementAndPseudo = getBindingElementAndPseudo;
Flags: needinfo?(poirot.alex)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Attachment #8907404 - Flags: review?(pbrosset) → review?(bgrinstead)
(Assignee)

Updated

a year ago
Attachment #8908400 - Flags: review?(poirot.alex)

Comment 9

a year ago
mozreview-review
Comment on attachment 8908400 [details]
Bug 1399314 - Move getBindingElementAndPseudo into shared/inspector/css-logic.js.

https://reviewboard.mozilla.org/r/180018/#review185366

Thanks
Attachment #8908400 - Flags: review?(poirot.alex) → review+
status-firefox57: --- → fix-optional

Comment 10

a year ago
mozreview-review
Comment on attachment 8907404 [details]
Bug 1399314 - Introdue CssLogic.getCSSStyleRules to get style rules for ::before and ::after pseudo elements handy.

https://reviewboard.mozilla.org/r/179094/#review186118

This does seem like the right thing to do, but I'd suggest that to avoid introducing this into future code we should instead add a new function `CSSLogic.getCSSStyleRules` that wraps up the call to `DOMUtils`:

    getCSSStyleRules(node) {
      let {bindingElement, pseudo} = this.getBindingElementAndPseudo(node);
      return domUtils.getCSSStyleRules(bindingElement, pseudo);
    }
    
And then update all existing references in our codebase of `DOMUtils.getCSSStyleRules` to use the new function (even in `_buildMatchedRules` - in that case we can get rid of the extra call to `getBindingElementAndPseudo` from the caller).
Attachment #8907404 - Flags: review?(bgrinstead)
Hi Hiro, I'm assigning the bug to you since you started to work on it and have received a few reviews. I hope you still want to finish it :)
Assignee: nobody → hikezoe
Priority: -- → P3
(Assignee)

Comment 12

a year ago
Thank you Patrick, the patch was piled up in my patch queue. (honestly I forgot it)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 16

a year ago
mozreview-review
Comment on attachment 8907404 [details]
Bug 1399314 - Introdue CssLogic.getCSSStyleRules to get style rules for ::before and ::after pseudo elements handy.

https://reviewboard.mozilla.org/r/179094/#review194440

Thanks
Attachment #8907404 - Flags: review?(bgrinstead) → review+

Comment 17

a year ago
Pushed by hikezoe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/be1d227ae5b3
Move getBindingElementAndPseudo into shared/inspector/css-logic.js. r=ochameau
https://hg.mozilla.org/integration/autoland/rev/eaa699653dc4
Introdue CssLogic.getCSSStyleRules to get style rules for ::before and ::after pseudo elements handy. r=bgrins
https://hg.mozilla.org/mozilla-central/rev/be1d227ae5b3
https://hg.mozilla.org/mozilla-central/rev/eaa699653dc4
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58

Updated

6 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.