691736 - Style inspector selector error with inline styles

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Steps to reproduce:
1. Go to http://mihaisucan.github.com/mozilla-work/test.html
2. Open the Web Console
3. inspectstyle(document.querySelector("#foobar .test"))
4. Expand the color properties matched selectors

The color properties matching CSS selectors should be displayed but "selector.humanReadableText(__element)" is displayed instead.

Comment 1

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Patch 1

Comment 2

[Mihai Sucan [:msucan]]

Comment on attachment 564543 [details] [diff] [review]
Patch 1

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

Thanks for opening the bug report and submitting the patch.

Giving r- because we need a test. Looking forward for the updated patch!

Comment 3

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Patch 2

Test added

Comment 4

[Mihai Sucan [:msucan]]

Comment on attachment 566765 [details] [diff] [review]
Patch 2

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

Test looks fine, only some nits. Please address them. Thank you!

::: browser/devtools/styleinspector/test/browser/browser_bug_691736_inline_selector.js
@@ +21,5 @@
> +    "  <p>hello world!</p>" +
> +    "  <div class=\"test\" style=\"color:blue; padding:5px;\">hello world!</div>" +
> +    "</div>" +
> +    "<div class=\"foobar\">foobar</div>" +
> +    "<div class=\"rule-link\"><a href=\"#\"></a></div>";

This is overly complex. Please only add an element with inline styles. Keep it as simple as possible.

@@ +25,5 @@
> +    "<div class=\"rule-link\"><a href=\"#\"></a></div>";
> +
> +  doc.title = "Style Inspector Selector Text Test";
> +  ok(window.StyleInspector, "StyleInspector exists");
> +  ok(StyleInspector.isEnabled, "style inspector preference is enabled");

Tests shouldn't fail if the Style Inspector is disabled.

@@ +38,5 @@
> +
> +  ok(stylePanel.isOpen(), "style inspector is open");
> +
> +  Services.obs.addObserver(SI_check, "StyleInspector-populated", false);
> +  SI_inspectNode();

Please merge SI_inspectNode() here.

Comment 5

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Patch 3

(In reply to Mihai Sucan [:msucan] from comment #4)
> Comment on attachment 566765 [details] [diff] [review] [diff] [details] [review]
> Patch 2
> 
> Review of attachment 566765 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> Test looks fine, only some nits. Please address them. Thank you!
> 
> :::
> browser/devtools/styleinspector/test/browser/
> browser_bug_691736_inline_selector.js
> @@ +21,5 @@
> > +    "  <p>hello world!</p>" +
> > +    "  <div class=\"test\" style=\"color:blue; padding:5px;\">hello world!</div>" +
> > +    "</div>" +
> > +    "<div class=\"foobar\">foobar</div>" +
> > +    "<div class=\"rule-link\"><a href=\"#\"></a></div>";
> 
> This is overly complex. Please only add an element with inline styles. Keep
> it as simple as possible.
> 

Done ... we still need the rule link though in order to prevent errors when getting the text.

> @@ +25,5 @@
> > +    "<div class=\"rule-link\"><a href=\"#\"></a></div>";
> > +
> > +  doc.title = "Style Inspector Selector Text Test";
> > +  ok(window.StyleInspector, "StyleInspector exists");
> > +  ok(StyleInspector.isEnabled, "style inspector preference is enabled");
> 
> Tests shouldn't fail if the Style Inspector is disabled.
> 

True, removed check

> @@ +38,5 @@
> > +
> > +  ok(stylePanel.isOpen(), "style inspector is open");
> > +
> > +  Services.obs.addObserver(SI_check, "StyleInspector-populated", false);
> > +  SI_inspectNode();
> 
> Please merge SI_inspectNode() here.

Done

Comment 6

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Patch 4

What is wrong with me today? I keep forgetting to hg refresh :o/

Comment 7

[Mihai Sucan [:msucan]]

(In reply to Michael Ratcliffe from comment #5)
> Created attachment 566839 [details] [diff] [review] [diff] [details] [review]
> Patch 3
> 
> > This is overly complex. Please only add an element with inline styles. Keep
> > it as simple as possible.
> > 
> 
> Done ... we still need the rule link though in order to prevent errors when
> getting the text.

That must not be needed. It seems to be a bug in the SelectorView_text() method. My quick gripe was with the missing this.win reference, and you fixed that. However, closer inspection of the code shows that the method is broken in several ways. Will post review. When I tested we did not have the InspectorUI Style button - I was only testing with the inspectstyle() command from the Web Console.

Going to explain the problem in the review.

> > @@ +25,5 @@
> > > +    "<div class=\"rule-link\"><a href=\"#\"></a></div>";
> > > +
> > > +  doc.title = "Style Inspector Selector Text Test";
> > > +  ok(window.StyleInspector, "StyleInspector exists");
> > > +  ok(StyleInspector.isEnabled, "style inspector preference is enabled");
> > 
> > Tests shouldn't fail if the Style Inspector is disabled.
> > 
> 
> True, removed check

Thank you! I saw you removed the check from other tests as well. You do not need to go back to other files and make unrelated changes. Still, you can keep this now that you added changes.

Comment 8

[Mihai Sucan [:msucan]]

Comment on attachment 566842 [details] [diff] [review]
Patch 4

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

Thanks for your updates!

The error you mentioned about the requirement of |.rule-link > a| brought my attention to more of that code in SelectorView_text() - I wanted to know why you say it causes errors. Found that the method is more broken than just the this.win property being undefined.

Please update the SelectorView_text() method as follows:

- make the sourceElement equality check against IUI.selection to be a strict equality check (from == to ===).
- bind to |this| the click event handler for .rule-link > a. The click didn't work.
- change the code such that the event handler is added even when sourceElement === IUI.selection (currently it's not).
- change the event handler to only call inspectNode() when sourceElement !== IUI.selection.

Unfortunately I have to revert the review to r- because the Style Panel integration into the Inspector has landed and we need this code to work properly. The Web Console use-cases didn't cover this issue...

Please also add into the inline_selector.js file a quick test for clicking on the selector source (.rule-link > a), to check if the inspectNode() method is called or not (check for both cases when .selection === sourceElement and when !==). You can avoid dealing with the InspectorUI code, just provide your own shim that checks it was called correctly. For example:

selectorView.win.InspectorUI = {
  selection: foo,
  inspectNode: function (aElement { /* your code */ },
)

Thank you!

::: browser/devtools/styleinspector/test/browser/browser_bug_691736_inline_selector.js
@@ +9,5 @@
> +
> +function createDocument()
> +{
> +  doc.body.innerHTML = "<div class=\"test\" style=\"color:blue;\"></div>" +
> +    "<div class=\"rule-link\"><a href=\"#\"></a></div>";

The .rule-link is not needed here. You can have:

  doc.body.innerHTML = "<div style='color:blue;'></div>";

(update the test to do querySelector('div'))

@@ +43,5 @@
> +  Services.obs.removeObserver(SI_check, "StyleInspector-populated", false);
> +  let propertyView = stylePanel.cssHtmlTree.propertyViews.color;
> +  let element = doc.querySelector(".test");
> +  try {
> +    let text = propertyView.matchedSelectorViews[0].text(element);

The direct call to text(element) requires you have .rule-link > a, which is ugly.

You should be able to do something like:

- expand the matched selectors of the color property view (set .matchedExpanded to true and call refreshMatchedSelectors()).
- retrieve the td.rule-text from propertyView.matchedSelectorTable (do a querySelector()).
- get the selector view reference, like you did (propertyView.matchedSelectorViews[0]).
- now do something like:
  is(td.textContent.trim(), selector.humanReadable(td).trim(), "selector text is correct")

And you put in try {} only the call to humanReadable().

Comment 9

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Patch 5

(In reply to Mihai Sucan [:msucan] from comment #8)
> Comment on attachment 566842 [details] [diff] [review] [diff] [details] [review]
> Patch 4
> 
> Review of attachment 566842 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> Thanks for your updates!
> 
> The error you mentioned about the requirement of |.rule-link > a| brought my
> attention to more of that code in SelectorView_text() - I wanted to know why
> you say it causes errors. Found that the method is more broken than just the
> this.win property being undefined.
> 
> Please update the SelectorView_text() method as follows:
> 
> - make the sourceElement equality check against IUI.selection to be a strict
> equality check (from == to ===).

There is no reason to select anything in the highlighter just because the CSS link is clicked. I have removed this code.

> - bind to |this| the click event handler for .rule-link > a. The click
> didn't work.

There is no reason to select anything in the highlighter just because the CSS link is clicked. I have removed this code.

> - change the code such that the event handler is added even when
> sourceElement === IUI.selection (currently it's not).

There is no reason to select anything in the highlighter just because the CSS link is clicked. I have removed this code.

> - change the event handler to only call inspectNode() when sourceElement !==
> IUI.selection.
> 

Done

> Unfortunately I have to revert the review to r- because the Style Panel
> integration into the Inspector has landed and we need this code to work
> properly. The Web Console use-cases didn't cover this issue...
> 

np

> Please also add into the inline_selector.js file a quick test for clicking
> on the selector source (.rule-link > a), to check if the inspectNode()
> method is called or not (check for both cases when .selection ===
> sourceElement and when !==). You can avoid dealing with the InspectorUI
> code, just provide your own shim that checks it was called correctly. For
> example:
> 
> selectorView.win.InspectorUI = {
>   selection: foo,
>   inspectNode: function (aElement { /* your code */ },
> )
> 
> Thank you!
> 

There is no reason to select anything in the highlighter just because the CSS link is clicked. I have removed this code.

> :::
> browser/devtools/styleinspector/test/browser/
> browser_bug_691736_inline_selector.js
> @@ +9,5 @@
> > +
> > +function createDocument()
> > +{
> > +  doc.body.innerHTML = "<div class=\"test\" style=\"color:blue;\"></div>" +
> > +    "<div class=\"rule-link\"><a href=\"#\"></a></div>";
> 
> The .rule-link is not needed here. You can have:
> 
>   doc.body.innerHTML = "<div style='color:blue;'></div>";
> 
> (update the test to do querySelector('div'))
> 

Done

> @@ +43,5 @@
> > +  Services.obs.removeObserver(SI_check, "StyleInspector-populated", false);
> > +  let propertyView = stylePanel.cssHtmlTree.propertyViews.color;
> > +  let element = doc.querySelector(".test");
> > +  try {
> > +    let text = propertyView.matchedSelectorViews[0].text(element);
> 
> The direct call to text(element) requires you have .rule-link > a, which is
> ugly.
> 
> You should be able to do something like:
> 
> - expand the matched selectors of the color property view (set
> .matchedExpanded to true and call refreshMatchedSelectors()).
> - retrieve the td.rule-text from propertyView.matchedSelectorTable (do a
> querySelector()).
> - get the selector view reference, like you did
> (propertyView.matchedSelectorViews[0]).
> - now do something like:
>   is(td.textContent.trim(), selector.humanReadable(td).trim(), "selector
> text is correct")
> 
> And you put in try {} only the call to humanReadable().

Done

Comment 10

[Mihai Sucan [:msucan]]

(In reply to Michael Ratcliffe from comment #9)
> Created attachment 567726 [details] [diff] [review] [diff] [details] [review]
> Patch 5
> 
> (In reply to Mihai Sucan [:msucan] from comment #8)
> > Comment on attachment 566842 [details] [diff] [review] [diff] [details] [review] [diff] [details] [review]
> > Patch 4
> > 
> > Review of attachment 566842 [details] [diff] [review] [diff] [details] [review] [diff] [details] [review]:
> > -----------------------------------------------------------------
> > 
> > Thanks for your updates!
> > 
> > The error you mentioned about the requirement of |.rule-link > a| brought my
> > attention to more of that code in SelectorView_text() - I wanted to know why
> > you say it causes errors. Found that the method is more broken than just the
> > this.win property being undefined.
> 
> There is no reason to select anything in the highlighter just because the
> CSS link is clicked. I have removed this code.

Why not?

Comment 11

[Mihai Sucan [:msucan]]

Comment on attachment 567726 [details] [diff] [review]
Patch 5

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

- The test fails because Bug 685902 landed and inline styles no longer show.

I would give the patch r+, but given the situation I am holding from giving it r-/r+.

Will rebase bug 692400 on top of fx-team so you can rebase this patch on top of 692400.

::: browser/devtools/styleinspector/test/browser/browser_bug_691736_inline_selector.js
@@ +12,5 @@
> +  doc.body.innerHTML = "<div style='color:blue;'></div>";
> +
> +  doc.title = "Style Inspector Selector Text Test";
> +  ok(window.StyleInspector, "StyleInspector exists");
> +  ok(StyleInspector.isEnabled, "style inspector preference is enabled");

Nit: this line shouldn't be back in this new file.

@@ +30,5 @@
> +}
> +
> +function SI_inspectNode()
> +{
> +  let span = doc.querySelector("div");

nit: s/span/div/

Comment 12

[Mihai Sucan [:msucan]]

Mike: the work Rob did for bug 685893 also had to touch this part of the code. This bug is now fixed.

Can you please update the patch to only add the test you wrote? That's useful for us to land, to be able to catch any future regressions.

Thank you! (no hurry!)

Comment 13

[Mihai Sucan [:msucan]]

Marking this bug as fixed (since bug 685893 landed). The test file has now been folded into the patch for bug 692400.