Closed Bug 1399314 Opened 7 years ago Closed 7 years ago

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

Categories

(DevTools :: Inspector, enhancement, P3)

Product:
DevTools
Component:
Inspector
Type:
enhancement

Tracking

(firefox57 fix-optional, firefox58 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 58
Tracking Flags:
Tracking Status
firefox57 --- fix-optional
firefox58 --- fixed

People

(Reporter: hiro, Assigned: hiro)

Details

Attachments

(2 files)

Bug 1399314 - Introdue CssLogic.getCSSStyleRules to get style rules for ::before and ::after pseudo elements handy.
59 bytes, text/x-review-board-request
bgrins
: review+
Details
Bug 1399314 - Move getBindingElementAndPseudo into shared/inspector/css-logic.js.
59 bytes, text/x-review-board-request
ochameau
: review+
Details 
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..
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
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)
Attachment #8907404 - Flags: review?(pbrosset) → review?(bgrinstead)
Attachment #8908400 - Flags: review?(poirot.alex)
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+
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
Thank you Patrick, the patch was piled up in my patch queue. (honestly I forgot it)
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+
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
Product: Firefox → DevTools
Component: Inspector: Computed → Inspector
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: