Closed Bug 1447736 Opened 6 years ago Closed 6 years ago

Rules view doesn't update if a parent or sibling of the selected node is updated

Categories

(DevTools :: Inspector: Rules, defect, P1)

Product:
DevTools
Component:
Inspector: Rules
Version:
57 Branch
Type:
defect

Tracking

(firefox61 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 61
Tracking Flags:
Tracking Status
firefox61 --- fixed

People

(Reporter: Harald, Assigned: pbro)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

Attachments

(5 files)

ScreenFlow.gif
3.61 MB, image/gif
Details
reduced_test_case.html
457 bytes, text/html
Details
Bug 1447736 - Refresh rule/computed-view even when parents and siblings change;
59 bytes, text/x-review-board-request
jdescottes
: review+
Details
additional_siblings_testcase.html
463 bytes, text/html
Details
testcase_sibling_of_parent.html
556 bytes, text/html
Details 
STR:
- [Could use a reduced test case]
- Open https://www.mozilla.org/en-US/firefox/58.0/firstrun/?v=j in a new profile
- Select text below headline (p.content)
- Refresh
- Wait for the animation, which is triggered with a data attribute (#scene[data-content="true"])
- Check rules panel

AR: Rules panel shows state before animation

ER: Data attribute doesn't seem to update the rules view.
Attached image ScreenFlow.gif — 
Screen flow that shows the problem
Attached file reduced_test_case.html — 
Adding reduced test case:

    <style>
      .test span{
        color: red
      }
    </style>
    <body>
      <div class="test" data-test="true">
        <span>Inspect me</span>
      </div>
      <button>click me</button>
      <script type="text/javascript">
        document.querySelector("button").addEventListener("click", function () {
          document.querySelector("div").classList.remove("test");  
        })
      </script>
    </body>

Issue only happens if a modification is made on a parent of the inspected element (here the div, while we inspect the span)
Attachment #8961068 - Attachment mime type: text/plain → text/html
Summary: Rules view doesn't update when data properties are added → Rules view doesn't update if a parent of the selected node is updated
Thanks for filing. It's quite frightening that we have not had a fix for this since the beginning of times. Cause I really think this has never worked.

By the way, it also would fail for sibling selectors.

Right now, we only update styles when a mutation occurs on the selected node, not when one of its ancestors or siblings change.

The logic for this is on the client-side in devtools/client/inspector/rules/rules.js, in this function:

 /**
   * When markup mutations occur, if an attribute of the selected node changes,
   * we need to refresh the view as that might change the node's styles.
   */
  onMutations: function(mutations) {
    for (let {type, target} of mutations) {
      if (target === this.inspector.selection.nodeFront &&
          type === "attributes") {
        this.refresh();
        break;
      }
    }
  },

This is incorrect as this should also work when ancestors or siblings change too.
Priority: -- → P1
Summary: Rules view doesn't update if a parent of the selected node is updated → Rules view doesn't update if a parent or sibling of the selected node is updated
I'm going to proceed with a rather simple fix that will make parent selectors work, but won't make sibling selectors work. It's better than nothing, and before doing the sibling part, I'd like us to spend more time thinking of a better system for this.
My fix is not going to change much of the logic, but I believe a more complete fix will need to change quite a lot more things. And I would like Brad's feedback on following idea:

Right now, we only listen to DOM mutations in the current document to know whether or not we should refresh the list of CSS rules displayed for a given nodes in the rule-view. We know now that that's more complex than initially thought, because when they happen, we need to look at whether they involved the selected node, one of its parents, or one of its siblings.
The question is the following: is there a step in the gecko rendering pipeline that we could hook into to do this instead of relying on DOM mutations. Is a restyle an indication that the list of CSS rules matching a given element has changed? Or is there another such type of indicator that, while currently buried in the platform, could be exposed to devtools?
Flags: needinfo?(bwerth)
(In reply to Patrick Brosset <:pbro> from comment #4)
> Is a restyle an indication that the list of CSS rules matching a
> given element has changed? Or is there another such type of indicator that,
> while currently buried in the platform, could be exposed to devtools?

Emilio could probably help us with this - emilio, is there a general place we could hook in for a callback to tell devtools that the list of rules that apply to an element has changed?

(I think "did selector-matching" is the right condition to watch for here, maybe - do you know if there's a good top-level spot when that definitely happened where we could notify devtools?  Or perhaps if that's too buried/abstracted-away in the stylo world, maybe we could at least set a flag for "did selector matching" there, and then we could check that flag up a few levels at the end of restyling and notify devtools if it's set, perhaps?)
Flags: needinfo?(bwerth) → needinfo?(emilio)
Does it matter which element changed?

It's tricky, because we only know that deep in the engine, and I can't think of a good way to expose this.

If "Any element's style changed" is enough, that's much easier. We have a restyle generation that we use for other sorts of stuff, so exposing it should be easy.
Flags: needinfo?(emilio) → needinfo?(pbrosset)
Yes in theory it does matter. The inspector in DevTools has a notion of "selected element", and we only want to refresh the list of CSS rules displayed if we need to. So if we had a more global event, we might end up refreshing too much (and in cases where elements styles are being mutated rapidly like during an animation, this might cause performance problems).

Our current solution isn't great either. We listen for DOM mutations and if they are targeting an attribute of the "selected element", then we refresh the rules. But that might not always mean the CSS rules matching the element did change either.

So, I guess we would really need to know when a certain element changed for it to be optimal.
Flags: needinfo?(pbrosset)
So, thinking a bit more about it, there are two things that can change what rules an element matches, DOM mutations (in the element or anywhere in the parent or previous sibling chain), and state changes (:hover and such).

Exposing this from the style engine is not great, I think, because the engine doesn't care about the styles of all the elements, and optimizes out elements in display: none subtrees. So whatever we may end up here would be incomplete.

So, how does devtools handle state changes (like :hover / :active / :checked / ...)?

We already expose an API to get the matched rules of an elements for devtools, which also handles undisplayed nodes and such.

Given devtools only cares for one element, can't devtools retain the rules used when it updates the panel view, and whenever any mutation happens, query them again to see if they change? That should be much cheaper than whatever solution we may end up with, given at most you only re-selector-match an extra element (and in most cases not even that, since if it's displayed we have optimizations to avoid that).
Flags: needinfo?(pbrosset)
(In reply to Emilio Cobos Álvarez [:emilio] from comment #8)
> So, thinking a bit more about it, there are two things that can change what
> rules an element matches, DOM mutations (in the element or anywhere in the
> parent or previous sibling chain), and state changes (:hover and such).
Media queries would be another one I guess. So when the window gets resized essentially.

> Exposing this from the style engine is not great, I think, because the
> engine doesn't care about the styles of all the elements, and optimizes out
> elements in display: none subtrees. So whatever we may end up here would be
> incomplete.
Good point. We still show CSS rules for elements in unrendered subtrees, so that would be a problem.

> So, how does devtools handle state changes (like :hover / :active / :checked
> / ...)?
It doesn't as far as I can tell.
So if you hover over a link that does have :hover styles applied to it, the inspector does not refresh to show the rule. And when you stop hovering, nothing changes either.
However we have options in devtools to force the :hover state (and others), so it's not a big problem I guess. Chrome does the same thing here.

For media queries, the inspector listens to window resize events to refresh the list of rules.
(and finally, for dom mutations, the inspector listens to them too, but we've already seen that before).

> We already expose an API to get the matched rules of an elements for
> devtools, which also handles undisplayed nodes and such.
> 
> Given devtools only cares for one element, can't devtools retain the rules
> used when it updates the panel view, and whenever any mutation happens,
> query them again to see if they change? That should be much cheaper than
> whatever solution we may end up with, given at most you only
> re-selector-match an extra element (and in most cases not even that, since
> if it's displayed we have optimizations to avoid that).
Sounds like the right thing to do. Thank you for investigating/thinking of other solutions.
Flags: needinfo?(pbrosset)
Assignee: nobody → pbrosset
Status: NEW → ASSIGNED
Attachment #8961740 - Attachment mime type: text/plain → text/html
Attachment #8961747 - Attachment filename: file_1447736.txt → testcase_sibling_of_parent.html
Attachment #8961747 - Attachment mime type: text/plain → text/html
Comment on attachment 8961668 [details]
Bug 1447736 - Refresh rule/computed-view even when parents and siblings change;

https://reviewboard.mozilla.org/r/230542/#review236088

Thanks for the patch, I like the mutualization and isolation in a new helper.
Found some edge cases not covered by the current implementation but we should handle them in followups.

::: devtools/client/inspector/shared/style-change-tracker.js:50
(Diff revision 1)
> +   * style change for the current node.
> +   */
> +  onMutations(mutations) {
> +    const canMutationImpactCurrentStyles = ({ type, target }) => {
> +      // Only attributes mutations are interesting here.
> +      if (type !== "attributes") {

For sibling rules, childList mutations can also be interesting. See the new attachment I added. 
https://bug1447736.bmoattachments.org/attachment.cgi?id=8961740

We can move this to a follow up.

::: devtools/client/inspector/shared/style-change-tracker.js:66
(Diff revision 1)
> +      // We can't know the order of nodes on the client-side without calling
> +      // walker.children, so don't attempt to check the previous or next element siblings.
> +      // It's good enough to know that one sibling changed.
> +      let parent = currentNode.parentNode();
> +      let siblings = parent.treeChildren();
> +      if (siblings.includes(target)) {

For some selectors we also need to update in case the mutation impacted a sibling of any ancestor, see my other testcase https://bugzilla.mozilla.org/attachment.cgi?id=8961747&action=edit . 

Again, this can be a follow up

::: devtools/client/inspector/shared/test/browser_styleinspector_refresh_when_style_changes.js:69
(Diff revision 1)
> +    await mutateDomAndWaitForViewChange(
> +      { target, className, shouldAdd }, inspector, tab, `${viewName}-view-refreshed`);
> +  }
> +}
> +
> +async function mutateDomAndWaitForViewChange(whatToMutate, inspector, tab, eventName) {

Since this toggles a class, we could have a less mysterious method name `toggleClassAndWaitForEvent`, and rename shouldAdd to force to match the spec.

::: devtools/client/inspector/shared/test/browser_styleinspector_refresh_when_style_changes.js:73
(Diff revision 1)
> +
> +async function mutateDomAndWaitForViewChange(whatToMutate, inspector, tab, eventName) {
> +  let onRefreshed = inspector.once(eventName);
> +
> +  await ContentTask.spawn(tab.linkedBrowser, whatToMutate,
> +    function({ target, className, shouldAdd }) {

Wow I never realized what the second argument of ContentTask.spawn actually was :)
Attachment #8961668 - Flags: review?(jdescottes) → review+
Blocks: 1449123
Thanks Julian. I filed bug 1449123 for the follow-ups identified in your review.
I had a bit of time yesterday to look at this again. My previous try build was showing an intermittent test. I tracked it down to it being a race condition, but the test itself was so hard to read and debug, that I had to end up rewriting it. I also fixed the race condition by making one of the head.js helpers wait for the right events, which it did not do before. So the problem sort of existed before, but didn't surface until I wrote this patch.

A new try push with my fix:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7aeebe46fb0514f7e675a2510095e304fe30a041

I'll ask for review again because the changes are quite substantial.
Attachment #8961668 - Flags: review+ → review?(jdescottes)
Comment on attachment 8961668 [details]
Bug 1447736 - Refresh rule/computed-view even when parents and siblings change;

https://reviewboard.mozilla.org/r/230542/#review237860

LGTM, but the modification to addProperty can easily timeout so I have a suggestion to improve it.

::: devtools/client/inspector/rules/test/head.js:279
(Diff revision 2)
>    let numOfProps = ruleEditor.rule.textProps.length;
>  
> +  // If we're adding a property to the element style, then that will also cause mutations
> +  // to the node, so make sure to wait for those too.
> +  let onMutations = Promise.resolve();
> +  if (ruleIndex === 0) {

The JSDoc for ruleIndex mentions: 

"if ruleIndex is 0, you might want to also listen to markupmutation events in your test since that's going to change the style attribute of the selected node."

That being said, I quickly looked for existing call sites and none of them seem to listen to markupmutations.

Should we update the JSDoc?

::: devtools/client/inspector/rules/test/head.js:280
(Diff revision 2)
>  
> +  // If we're adding a property to the element style, then that will also cause mutations
> +  // to the node, so make sure to wait for those too.
> +  let onMutations = Promise.resolve();
> +  if (ruleIndex === 0) {
> +    onMutations = waitForNEvents(view.inspector.walker, "mutations", 2);

I recently had an issue when waiting for several mutations like this. Mutations can be throttled and grouped on the server. I tried quickly to modify a test to perform two `await addProperty();` one after the other and I hit this edge case so I think we should fix the code here.

For example you can fix it by doing:

  let onMutations = new Promise(r => {
    if (ruleIndex !== 0) {
      r();
    }
    let receivedMutations = 0;
    view.inspector.walker.on("mutations", function onWalkerMutations(mutations) {
      receivedMutations += mutations.length;
      if (receivedMutations >= 2) {
        view.inspector.walker.off("mutations", onWalkerMutations);
        r();
      }
    });
  });

::: devtools/client/inspector/rules/test/head.js:280
(Diff revision 2)
>  
> +  // If we're adding a property to the element style, then that will also cause mutations
> +  // to the node, so make sure to wait for those too.
> +  let onMutations = Promise.resolve();
> +  if (ruleIndex === 0) {
> +    onMutations = waitForNEvents(view.inspector.walker, "mutations", 2);

Can we add a comment explaining why 2? Are there tricky edge cases where this would not happen?

::: devtools/client/inspector/rules/test/head.js:311
(Diff revision 2)
>  
>    let onValueAdded = view.once("ruleview-changed");
>    EventUtils.synthesizeKey(commitValueWith, {}, view.styleWindow);
>    await onValueAdded;
>  
> +  await onMutations;

Can we add an info here to say we are waiting for mutations?
Attachment #8961668 - Flags: review?(jdescottes) → review+
Thanks for the tip Julian. I'll add the code in and address your other comments.
Pushed by pbrosset@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c2b1e0b5cc31
Refresh rule/computed-view even when parents and siblings change; r=jdescottes
https://hg.mozilla.org/mozilla-central/rev/c2b1e0b5cc31
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox61: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: