>>> My Info: Win7_64, Nightly 52, 32bit, ID 20161028030204 (2016-10-28) STR_1: 1. Open url data:text/html,<div style="cursor:pointer">A<div style="cursor:pointer">B<a>Cursor 2. Open inspector -> ruleview, inspect <a> element 3. In the section "Inherited from" choose any element, click on highlight button near text "element" AR: <a> element is highlighted ER: Either X or Y X) Highlight button should correctly highlight chosen <div> element Y) There should be no highlight button near "element" Note: This was implemented in broken state between Nightly 2016-05-26 and Nightly 2016-10-28

Narrowed inbound regression window from [51a0d74d, 7b8d79f7] (3 revisions) to [51a0d74d, 182fd3e6] (2 revisions) (~1 steps left) Oh noes, no (more) inbound revisions :( Last good revision: 51a0d74d0b05d882d18e96cc3bcec6dcae1cd261 First bad revision: 182fd3e63603ca64d166a08c86a3347af1ff7385 Pushlog: https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=51a0d74d0b05d882d18e96cc3bcec6dcae1cd261&tochange=182fd3e63603ca64d166a08c86a3347af1ff7385 Bug 1297383 is most likely the cause. This is not a regression as the issue is reproducible ever since the feature was implemented. Sébastien, could you please take a look at this?

Sure, I will do this tomorrow.

Ok, So, the problem is in devtools/client/inspector/rules/views/rule-editor.js, when we have `this.rule.domRule.type !== ELEMENT_STYLE`, `let selector = this.ruleView.inspector.selectionCssSelector;` give the selector of the element. But if `rule.inherited !== null`, the selector should be changed. I think the best solution is "X) Highlight button should correctly highlight chosen <div> element", (the "Y) There should be no highlight button near "element"" is easy to implement, but I think it's not the best solution)

Inspector bug triage (filter on CLIMBING SHOES).

@pbro: I'm wondering how can I easily get the selector of the correct element. I tried to deduce the selector from rule.inherited, but I don't think it's the correct way. Have a nice day,

(In reply to Sébastien Blin [:sblin] [:amarok] from comment #5) > @pbro: > I'm wondering how can I easily get the selector of the correct element. I > tried to deduce the selector from rule.inherited, but I don't think it's the > correct way. > > Have a nice day, The problem is this.rule.inherited doesn't contains informations like parentNode or ownerDocument (So I can't use findCssSelector(ele) from css-logic).

I looked at the code for a while and found that it's going to be a little more complicated than I originally thought. findCssSelector only works with real DOM elements. However, the part of the code that deals with displaying inherited rules runs in the devtools front-end process. This process does not have access to the web page being inspected at all. This is why this.inherited doesn't contain information about the DOM node. DevTools is split in 2 parts: server-side and client-side. The server has access to the web page. It has access to the real DOM nodes, the stylesheets, etc. The client is where the UI is displayed. It has no access to the web page. It only deals with the UI and issues requests to the server whenever it needs information from it. That is because DevTools can be used to debug remote devices for example. So, what happens when a new node is selected in the inspector, is the client gets information from the server about this node (but it doesn't get access to the node itself). This information is in the form of a NodeFront instance. NodeFront is a class that has many properties and methods, one of which is getUniqueSelector(). So when a node is selected in the inspector, one of the first things that happens is getUniqueSelector is called, and that returns (asynchronously, because that's a server-side request) the CSS selector that can be used to highlight the node. Now, the inherited rules apply to other DOM nodes, so in theory we should also call getUniqueSelector for these other nodes. In reality we don't and that's the reason for this bug. In client/inspector/rules/views/rule-editor.js in the _create function, we come up with a selector string: let selector = this.rule.domRule.selectors ? this.rule.domRule.selectors.join(", ") : this.ruleView.inspector.selectionCssSelector; So, if the rule is an inline style (i.e. the domRule.selectors array doesn't exist) we use this.ruleView.inspector.selectionCssSelector. This property's value is the one we got when calling getUniqueSelector() when the current node was selected. That's the bug because in that case, we're not displaying an inline style for the current node but for a node which rule is being inherited. So, if we're displaying an inherited rule -> if (this.rule.inherited) Then we should get its unique selector: -> this.rule.inherited.getUniqueSelector() The hard part is that this is an asynchronous call, and the rest of this code expects the 'selector' variable to be defined synchronously. One way is to wrap the whole thing in: Task.spawn(function* () { }.bind(this)); This way you can write the code in a more readable manner. Just to summarize, there will be now 3 cases: 1. if this.rule.domRule.selectors exists, then the rule is a "normal" one, with selectors. In this case selector is simply this.rule.domRule.selectors.join(", ") 2. if this.rule.domRule.selectors does not exist *and* this.rule.inherited does not exist. Then we are dealing with the inline style of the *current* node. In this case, selector is simply this.ruleView.inspector.selectionCssSelector 3. if this.rule.domRule.selectors does not exist *and* this.rule.inherited *does* exist. Then we are dealing with the inline style of another element (an inherited inline style). In this case, you need to call this.rule.inherited.getUniqueSelector() to get the right selector. If you use the Task.spawn that I mentioned before, you can just do: selector = yield this.rule.inherited.getUniqueSelector(); Now, there's another bug here. As we discussed on IRC, there's only one "inherited from" section in the UI, where there should really be 2 (at least with the provided test scenario). The bug here is that we just compare the <something> part in "inherited from <something>" before creating a new section. In the test scenario provided here, that's <div> both times, so there's only one section. But really, these are 2 different divs, so we should have 2 sections. The bug is in client/inspector/rules/rules.js in _createEditors When we have an inheritedSource, we create a new element to separate the rules, but we don't do it if: inheritedSource !== lastInheritedSource And here we just end up comparing "<div>" to "<div>". Instead, we should compare unique things. In this case, the obvious unique ID we can use is the CSS Rule ActorID. That's a unique ID that identifies CSS rules. Its value is in rule.domRule.actorID So, instead of lastInheritedSource, we could use something called lastInheritedRuleID instead and give it the value rule.domRule.actorID. I hope this helps. Sorry for the delay.

(In reply to Patrick Brosset <:pbro> from comment #7) > Now, there's another bug here. As we discussed on IRC, there's only one > "inherited from" section in the UI, where there should really be 2 (at least > with the provided test scenario). > The bug here is that we just compare the <something> part in "inherited from > <something>" before creating a new section. > In the test scenario provided here, that's <div> both times, so there's only > one section. But really, these are 2 different divs, so we should have 2 > sections. I just noticed bug 1327767 which is about this exact problem. So forget about this, better have separate bugs for these 2 issues.

Thanks for the explanation. Indeed, `this.rule.inherited.getUniqueSelector()` in an async task works :)

Comment on attachment 8825217 [details] [diff] [review] bug_1327675.patch Review of attachment 8825217 [details] [diff] [review]: ----------------------------------------------------------------- Test looks great, thanks for this! Just one last comment about the main code change. See below. ::: devtools/client/inspector/rules/test/browser_rules_selector-highlighter_05.js @@ +40,5 @@ > + info("Checking that the right NodeFront reference and options are passed"); > + yield selectNode("a", inspector); > + > + let textProp = yield addProperty(view, 0, "background-color", "white"); > + is(textProp.value, "white", "Text prop should have been changed."); These 2 lines seem unnecessary for this test. ::: devtools/client/inspector/rules/views/rule-editor.js @@ +152,4 @@ > let selector = this.rule.domRule.selectors > ? this.rule.domRule.selectors.join(", ") > : this.ruleView.inspector.selectionCssSelector; > + // Bug 1327675: the selector is wrong if the rule is inherited for an inline style This seems to work almost perfectly, thanks! I do see 2 problems with it however: - First, I'm afraid the logic to set the selector is getting complex and hard to follow for someone reading the code for the first time. Before your change, there were 2 cases, and no comments, so this was already a bit hard. After your change, there are still these 2 cases, unchanged, and then a third condition separate from that. I think we should take the opportunity of your change to rewrite the whole condition as a if/elseif/else kind of thing, so that it's easier to understand what the 3 cases are and we can also add comments for each of them. - Secondly, the variable selector is used synchronously just later, when creating selectorHighlighter. However, the Task.spawn is asynchronous, so it will run after. Which means selector won't have the right value when we selectorHighlighter is initialized. This causes a bug that you can see with this: - open the URL provided in comment 0 - select one of the divs (any of the 2) - click on the selector icon in the rule-view, on the element{} rule - the DIV gets highlighted in the page - select the <a> element in the inspector ==> The selector icon next to the inherited element{} rule is *not* highlighted, while it should. Let me suggest this instead: - let's wrap the entire block of code that's inside if (this.rule.domRule.type !== CSSRule.KEYFRAME_RULE) in a Task.spawn. This way, this whole block is executed asynchrously from the rest, but at least, within that block, we can yield on the things we want to block. - then inside the Task, separate the 3 known cases into if/else if/else with comments making it clearer what cases is being dealt with: if (this.rule.domRule.type !== CSSRule.KEYFRAME_RULE) { Task.spawn(function* () { let selector; if (this.rule.domRule.selectors) { // This is a "normal" rule with a selector. selector = this.rule.domRule.selectors.join(", "); } else if (this.rule.inherited) { // This is an inline style from an inherited rule. Need to resolve the unique // selector from the node which rule this is inherited from. selector = yield this.rule.inherited.getUniqueSelector(); } else { // This is an inline style from the current node. selector = this.ruleView.inspector.selectionCssSelector; } //etc.... }.bind(this)); } This way selector has the right value when we use it later, no matter if it was defined synchronously or async.

Woops, didn't see the 2 lines in my test. Just removed it but the test doesn't pass anymore. It's normal, because when I do a `view.styleDocument.querySelectorAll(".ruleview-selectorhighlighter");` all selectorhightlighter are not created. So I need to wait until all icons are created. Is there a signal or something that I can add just before `let icons = view.styleDocument.querySelectorAll(".ruleview-selectorhighlighter");`?

(In reply to Sébastien Blin [:sblin] [:amarok] from comment #11) > the test doesn't pass anymore. It's normal, because when I do a > `view.styleDocument.querySelectorAll(".ruleview-selectorhighlighter");` all > selectorhightlighter are not created. So I need to wait until all icons are > created. Is there a signal or something that I can add? You are right, I didn't see this coming. There isn't an event for waiting for this yet. I guess we could add one though. It would only be for tests, but that wouldn't be the first time we added a test-only event (adding production code only for the sake of tests isn't something great, but sometimes we have no choice, given the async nature of our code, we jut need hooks in some places so the tests know when things are done). The only other solution I can think of is polling the DOM to see when the icons have been created, but that's even worse.

Comment on attachment 8826718 [details] [diff] [review] bug_1327675.patch Review of attachment 8826718 [details] [diff] [review]: ----------------------------------------------------------------- ::: devtools/client/inspector/rules/test/browser_rules_selector-highlighter_05.js @@ +44,5 @@ > + > + info("Checking that the right NodeFront reference and options are passed"); > + yield selectNode("a", inspector); > + > + let icons = view.styleDocument.querySelectorAll(".ruleview-selectorhighlighter"); I think you should use getRuleViewSelectorHighlighterIcon here instead of querySelectorAll. See my next comment about adding the new event. You will need to call getRuleViewSelectorHighlighterIcon 3 times to get all icons though. ::: devtools/client/inspector/rules/views/rule-editor.js @@ +171,5 @@ > + title: l10n("rule.selectorHighlighter.tooltip") > + }); > + selectorHighlighter.addEventListener("click", () => { > + this.ruleView.toggleSelectorHighlighter(selectorHighlighter, selector); > + }); At the end of the task, you could add something like this: this.uniqueSelector = selector; this.emit("selector-icon-created"); This way, the tests that test this part can do: let editor = getRuleViewRuleEditor(view, index); if (!editor.uniqueSelector) { yield once(editor, "selector-icon-created"); } And then be guaranteed that the icon exists and the selector has been resolved. Note that we have several tests that use getRuleViewSelectorHighlighterIcon. This is a helper function that gets the icon. These tests are going to have to wait for the event now. So you will need to update getRuleViewSelectorHighlighterIcon as part of your patch and then change all the tests that rely on it: 1. make sure getRuleViewSelectorHighlighterIcon is declared just once (it's apparently duplicated between devtools\client\inspector\rules\test\head.js and devtools\client\inspector\shared\test\head.js, it should only be in the shared head.js file) 2. update getRuleViewSelectorHighlighterIcon so that it becomes async and has the piece of code I gave above. That means you'll need to wrap it in a Task.async. 3. update all the tests that use getRuleViewSelectorHighlighterIcon so they use 'yield' to call it.

It's not finish (at all) but: 1. There is a lot of re-definitions between the rules head.js and the shared head.js. I think I removed all re-def. 2. I think I don't understand what you want. I've got something like: ```js var getRuleViewSelectorHighlighterIcon = Task.async(function* (view, selectorText) { let rule = getRuleViewRule(view, selectorText); /** TODO wait for "selector-icon-created" let editor = getRuleViewRuleEditor(view, index); if (!editor.uniqueSelector) { yield once(editor, "selector-icon-created"); } */ return rule.querySelector(".ruleview-selectorhighlighter"); }); ``` But we don't have the index, just the selector. Do I need to update all `getRuleViewSelectorHighlighterIcon` and add an index parameter. Or this part is somewhere else? 3. I will do it when 2. will work.

(In reply to Sébastien Blin [:sblin] [:amarok] from comment #14) > 2. I think I don't understand what you want. I've got something like: > > ```js > var getRuleViewSelectorHighlighterIcon = Task.async(function* (view, > selectorText) { > let rule = getRuleViewRule(view, selectorText); > > /** > TODO wait for "selector-icon-created" > let editor = getRuleViewRuleEditor(view, index); > if (!editor.uniqueSelector) { > yield once(editor, "selector-icon-created"); > } > */ > > return rule.querySelector(".ruleview-selectorhighlighter"); > }); > ``` > But we don't have the index, just the selector. Do I need to update all > `getRuleViewSelectorHighlighterIcon` and add an index parameter. Or this > part is somewhere else? Ah I see. In fact, you might not need getRuleViewRuleEditor here. You can do: let rule = getRuleViewRule(view, selectorText); let editor = rule._ruleEditor; And then the rest!

Moving test code around is not particularly interesting, so I thought I'd help you on that part (the part 1. in your last comment). In fact, the inspector/rules/test/head.js and inspector/shared/test/head.js don't depend on each other. They have duplicated code, but that code needs to be put in common somewhere else that both of these files depend on. Turns out they both include inspector/test/head.js, which itself includes inspector/test/shared-head.js So I moved all of the duplicated code in this shared-head.js. Feel free to use this patch, and then base your own patch on top of it (if you use mercurial MQ, you can have several patches in your queue on top of eachother. If you use Mercurial alone, then you could import this patch and then rebase yours on top of it. Or you can decide to redo this yourself if you want to, but hopefully this is helpeful).

Yeah I use MQ. So, I used your patch (thanks). I updated all tests with `yield getRuleViewSelectorHighlighterIcon`. Finally, I add a new parameter to getRuleViewRule(view, "element") because this function could only get the first rule and not inherited rules (so I add an offset).

Comment on attachment 8828979 [details] [diff] [review] bug_1327675.patch Review of attachment 8828979 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me. I just have a couple of comments about some variables names and some jsdoc, but I don't think this should block R+. Please re-upload a patch with your last changes and mark it R+ yourself. Can you push to try on your own or do you need someone to do it for you? ::: devtools/client/inspector/test/shared-head.js @@ +298,5 @@ > + return onEdit; > +}); > + > +/** > + * Get the Xe DOMNode for a css rule in the rule-view that corresponds to the I would keep this comment as simple as it was before. It's hard to understand what offset is just by reading this. So let's keep: Get the DOMNode for a css rule in the rule-view that corresponds to the given selector. @@ +307,5 @@ > + * @param {String} selectorText > + * The selector in the rule-view for which the rule > + * object is wanted > + * @param {Integer} offset > + * The position in the rule-view. Begin at 0. And here, let's make the offset clearer: @param {Number} index If there are more than 1 rule with the same selector, you may pass a index to determine which of the rules you want. @@ +321,5 @@ > + if (offset == pos) { > + rule = r; > + break; > + } > + pos += 1; pos ++ @@ +399,5 @@ > + * @param {CssRuleView} view > + * The instance of the rule-view panel > + * @param {String} selectorText > + * The selector in the rule-view to look for > + * @param {Integer} offset Same comment here, let's rename offset to index, and change the comment to say something like: "if there are more than 1 rule with the same selector, use this index to determine which one should be retrieved. Defaults to 0"

I can't push, I don't have the level 1.

There you go: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ca4a09a4a0ad3f82efe89aa1fdc61f52510ac909 Let's wait a bit until that try push completes. If it's green, we'll ask for someone to land your patch. Thanks for your work on this so far!

Hi, we can see some connection closed due to the connection for the highlighter icon. So I add yield getRuleViewSelectorHighlighterIcon() in 2 tests. Can you push this?

Right, I did see these test failures. I'll keep the NI? flag now as a reminder to look at the failures and at your new patch in more details next week. I'll push to try then. Thanks!

Thanks for the new patch. Your changes look good. I've also fixed ESLint issues. Here's a new try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c6f38805788f3701b8f5153b1a16ae61d198ce88

Try push from my last comment is green. There was just one last ESLint error I had missed, and I also corrected the commit message, so here's a new patch. Let's check this in. Thanks for working on this!

Pushed by cbook@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/85d33b74434a Correctly hightlight nodes for inherited rules. r=pbro

I had to back this out for devtools failures like https://treeherder.mozilla.org/logviewer.html#?job_id=73299881&repo=mozilla-inbound https://hg.mozilla.org/integration/mozilla-inbound/rev/895ddf282ead4d4533313c43b7cfa0aa218981e3

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/bb3edf240bb0 Correctly hightlight nodes for inherited rules. r=pbro

Backed out for eslint failure: https://hg.mozilla.org/integration/mozilla-inbound/rev/fa2bc59c9b3736f4ef4a38b7c03665a6f2966acb Push with failure: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=18d1694428a0954829980074cd63ded3ad02c0c6 Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=73702483&repo=mozilla-inbound TEST-UNEXPECTED-ERROR | /home/worker/checkouts/gecko/devtools/client/inspector/rules/test/browser_rules_selector-highlighter_05.js:50:40 | Trailing spaces not allowed. (no-trailing-spaces) TEST-UNEXPECTED-ERROR | /home/worker/checkouts/gecko/devtools/client/inspector/test/shared-head.js:202:13 | 'defer' is not defined. (no-undef) TEST-UNEXPECTED-ERROR | /home/worker/checkouts/gecko/devtools/client/inspector/test/shared-head.js:237:10 | 'promise' is not defined. (no-undef) TEST-UNEXPECTED-ERROR | /home/worker/checkouts/gecko/devtools/client/inspector/test/shared-head.js:296:16 | 'inplaceEditor' is not defined. (no-undef) TEST-UNEXPECTED-ERROR | /home/worker/checkouts/gecko/devtools/client/inspector/test/shared-head.js:326:7 | Unexpected space before unary operator '++'. (space-unary-ops) TEST-UNEXPECTED-ERROR | /home/worker/checkouts/gecko/devtools/client/inspector/test/shared-head.js:465:6 | 'inplaceEditor' is not defined. (no-undef) TEST-UNEXPECTED-ERROR | /home/worker/checkouts/gecko/devtools/client/inspector/test/shared-head.js:513:3 | 'synthesizeKeys' is not defined. (no-undef)

My bad... didn't see the previous diff from pbro. Should be better.

Comment on attachment 8833318 [details] [diff] [review] bug1327675.patch You added a new test browser_rules_selector-highlighter_05.js to the moz.build file but the new test itself is missing from the patch. Did you forget to run |hg add|?

...

Please stick to using checkin-needed, it plays more nicely with the various bug marking tools.

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/673c3bd921c7 Correctly hightlight nodes for inherited rules. r=pbro

Build ID: 20170228030203 User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:54.0) Gecko/20100101 Firefox/54.0 Verified as fixed on Firefox Nightly 54.0a1 on Windows 10 x 64, Mac OS X 10.11 and Ubuntu 16.04 x64.