1163183 - Show HTML5 Forms pseudo elements in the rule view

Status: VERIFIED FIXED

(DevTools :: Inspector, defect)

Comment 1

[Jen Fong-Adwent [:ednapiranha]]

Current list borrowed from here https://dxr.mozilla.org/mozilla-central/source/layout/style/nsCSSPseudoElementList.h#53

Comment 2

[Brian Grinstead [:bgrins]]

We just need to add the new pseudo elements to the whitelist here: https://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/styles.js#30

Comment 3

[Brian Grinstead [:bgrins]]

Found this when looking around: https://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/styleinspector/css-logic.js#1537.  I'm not positive where else it is in use, but we should probably share this list so we don't have to keep multiple copies in sync with nsCSSPseudoElementList.h. 

I'm thinking we could move it to CssLogic.pseudoElements and then reference that from the styles.js file

Comment 4

[Jen Fong-Adwent [:ednapiranha]]

Attachment: Bug1163183.patch

First pass at this patch - let me know if that looks good or if we need to move anything around. 

I also wasn't able to figure out where the tests should go (if any are present)? But I did run a try just to see how that would turn out https://treeherder.mozilla.org/#/jobs?repo=try&revision=5968444dd2f4

Comment 5

[Jen Fong-Adwent [:ednapiranha]]

Comment on attachment 8604712 [details] [diff] [review]
Bug1163183.patch

Actually found some of the tests through try. I'll submit an updated patch for feedback after.

Comment 6

[Brian Grinstead [:bgrins]]

btw you can use a shorthand to create a set like this: `new Set(["foo", "bar"])`

Comment 7

[Jen Fong-Adwent [:ednapiranha]]

Attachment: Bug163183.patch

I got this working but had to have an ignore list for a couple of pseudo elements which caused errors in tests and one in rendering (moz-color-swatch). Not sure why these ones in particular are an exception - should we leave it as is or is there a way around it?

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=137775c2508f

Comment 8

[Brian Grinstead [:bgrins]]

(In reply to Jen Fong-Adwent [:ednapiranha] from comment #7)
> Created attachment 8605877 [details] [diff] [review]
> Bug163183.patch
> 
> I got this working but had to have an ignore list for a couple of pseudo
> elements which caused errors in tests and one in rendering
> (moz-color-swatch). Not sure why these ones in particular are an exception -
> should we leave it as is or is there a way around it?
> 

What kind of errors?  If it's things like:

browser_ruleview_user-agent-styles.js | Should have correct number of rules (9) - Got 8, expected 9

then most likely we just need to update the tests to expect to see more rules - after all, we are going to be showing more rules with this patch applied, right?

Comment 9

[Brian Grinstead [:bgrins]]

Comment on attachment 8605877 [details] [diff] [review]
Bug163183.patch

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

::: toolkit/devtools/server/actors/styles.js
@@ +27,5 @@
>  // but no associated rule) we fake a rule with the following style id.
>  const ELEMENT_STYLE = 100;
>  exports.ELEMENT_STYLE = ELEMENT_STYLE;
>  
> +const PSEUDO_ELEMENTS_IGNORE = [

See Comment 8.  But assuming we still need to omit certain pseudo elements from the rule view, we could handle it like this: Instead of exporting PSEUDO_ELEMENT_ARRAY from the other file we could just export PSEUDO_ELEMENT_SET and then remove the relevant elements from the set here:

set.delete("-moz-color-swatch");
set.delete("-moz-list-bullet");
set.delete("-moz-list-number");
set.delete("-moz-meter-bar");
set.delete("-moz-progress-bar");
const PSEUDO_ELEMENTS = Array.from(set);

Comment 10

[Jen Fong-Adwent [:ednapiranha]]

(In reply to Brian Grinstead [:bgrins] from comment #8)
> (In reply to Jen Fong-Adwent [:ednapiranha] from comment #7)
> > Created attachment 8605877 [details] [diff] [review]
> > Bug163183.patch
> > 
> > I got this working but had to have an ignore list for a couple of pseudo
> > elements which caused errors in tests and one in rendering
> > (moz-color-swatch). Not sure why these ones in particular are an exception -
> > should we leave it as is or is there a way around it?
> > 
> 
> What kind of errors?  If it's things like:
> 
> browser_ruleview_user-agent-styles.js | Should have correct number of rules
> (9) - Got 8, expected 9
> 
> then most likely we just need to update the tests to expect to see more
> rules - after all, we are going to be showing more rules with this patch
> applied, right?

In browser_ruleview_user-agent-styles.js if I add those elements the test compareAppliedStylesWithUI fails with errors like:

TEST-UNEXPECTED-FAIL | browser/devtools/styleinspector/test/browser_ruleview_user-agent-styles.js | No inline styles for ua styles

TEST-UNEXPECTED-FAIL | browser/devtools/styleinspector/test/browser_ruleview_user-agent-styles.js | Editor isEditable opposite of UA (true) - Got true, expected false

TEST-UNEXPECTED-FAIL | browser/devtools/styleinspector/test/browser_ruleview_user-agent-styles.js | Same isSystem (true) - Got undefined, expected true

Comment 11

[Brian Grinstead [:bgrins]]

(In reply to Jen Fong-Adwent [:ednapiranha] from comment #10)
> (In reply to Brian Grinstead [:bgrins] from comment #8)
> > (In reply to Jen Fong-Adwent [:ednapiranha] from comment #7)
> > > Created attachment 8605877 [details] [diff] [review]
> > > Bug163183.patch
> > > 
> > > I got this working but had to have an ignore list for a couple of pseudo
> > > elements which caused errors in tests and one in rendering
> > > (moz-color-swatch). Not sure why these ones in particular are an exception -
> > > should we leave it as is or is there a way around it?
> > > 
> > 
> > What kind of errors?  If it's things like:
> > 
> > browser_ruleview_user-agent-styles.js | Should have correct number of rules
> > (9) - Got 8, expected 9
> > 
> > then most likely we just need to update the tests to expect to see more
> > rules - after all, we are going to be showing more rules with this patch
> > applied, right?
> 
> In browser_ruleview_user-agent-styles.js if I add those elements the test
> compareAppliedStylesWithUI fails with errors like:
> 
> TEST-UNEXPECTED-FAIL |
> browser/devtools/styleinspector/test/browser_ruleview_user-agent-styles.js |
> No inline styles for ua styles
> 
> TEST-UNEXPECTED-FAIL |
> browser/devtools/styleinspector/test/browser_ruleview_user-agent-styles.js |
> Editor isEditable opposite of UA (true) - Got true, expected false
> 
> TEST-UNEXPECTED-FAIL |
> browser/devtools/styleinspector/test/browser_ruleview_user-agent-styles.js |
> Same isSystem (true) - Got undefined, expected true

Hm, seems like the UI should be consistent with the return value from getApplied - if not maybe this is triggering an actual bug with the frontend that we should investigate.

Lots of times if there is a problem with the test you can stick a `yield promise.defer().promise` at the bottom of the task and then click around in the UI to figure out exactly what is going on.  Or you can stick a debugger statement in the test and the run the command as `./mach mochitest-devtools path/to/test --jsdebugger`.

Comment 12

[Jen Fong-Adwent [:ednapiranha]]

Attachment: Bug1163183.patch

Finally figured out that the -moz pseudo elements needed :: instead of : for browser_ruleview_user-agent-styles.js to pass.

The one test failing now that the double colons have been added is browser_ruleview_edit-selector_02.js for the reference to ::first-letter - if I change it back to single colon for first-letter in styles.js, it passes. If I use double colons, the tests fail in getRuleViewRuleEditor (head.js) as it returns one index short of the expectation. The error in the test is:

TEST-UNEXPECTED-FAIL | browser/devtools/styleinspector/test/browser_ruleview_edit-selector_02.js | Uncaught exception - at chrome://mochitests/content/browser/browser/devtools/styleinspector/test/head.js:774 - TypeError: view.element.children[childrenIndex] is undefined

Is there something relying on the single colon higher up on the chain? Or do we have to put in conditional checks for some CSS2 pseudo elements in style.js?

Other than that issue, let me know if anything in this patch should be changed - thanks!

Comment 13

[Brian Grinstead [:bgrins]]

Comment on attachment 8612958 [details] [diff] [review]
Bug1163183.patch

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

Looks good, and as far as ':' vs '::' I'd say let's just go with what works for now (see inline comments)

::: browser/devtools/styleinspector/test/browser_ruleview_user-agent-styles.js
@@ +79,4 @@
>    yield selectNode("blockquote", inspector);
>    yield compareAppliedStylesWithUI(inspector, view, "ua");
>  
> +  userRules = view._elementStyle.rules.filter(rule=>rule.editor.isEditable);

This is mostly due to this test file not being super well organized to start with, but it looks like a pattern could be pulled out here so that we could keep an array of test data:

let TEST_DATA_UA = [
  { selector: "blockquote", numUserRules: 1 },
  { selector: "input[type=range]", numUserRules: 1 },
  { selector: "input[type=number]", numUserRules: 1 },
  ...
];

Then loop through it:

for (let data of TEST_DATA_UA) {
  yield selectNode("blockquote", inspector);
  yield compareAppliedStylesWithUI(inspector, view, "ua");
  let userRules = view._elementStyle.rules.filter(rule=>rule.editor.isEditable);
  let uaRules = view._elementStyle.rules.filter(rule=>!rule.editor.isEditable);
  is (userRules.length, 1, "Correct number of user rules");
  ok (uaRules.length > 0, "Has UA rules");
}

And do the something very similar for userAgentStylesNotVisible.

Note that this may require pulling the last check (on the "a" element where it does special stuff with actually comparing some of the styles) into a separate test function.  But that would probably actually help simplify things in this test.

It's possible to do this as a follow up if it becomes complex to handle it here.

::: toolkit/devtools/server/actors/styles.js
@@ +27,5 @@
>  // but no associated rule) we fake a rule with the following style id.
>  const ELEMENT_STYLE = 100;
>  exports.ELEMENT_STYLE = ELEMENT_STYLE;
>  
> +let set = PSEUDO_ELEMENT_SET;

I don't think this actually needs to be reassigned (even though it's a const you can still do the set operations on it)

@@ +564,5 @@
>  
> +    let rule = {
> +      rule: elementStyle,
> +      pseudoElement: null,
> +      isSystem: false

Maybe also set inherited: false here just to be clear

@@ +574,5 @@
>      }
>  
>      // Now any inherited styles
>      if (showInheritedStyles) {
> +      rule.inherited = inherited;

rule.inherited = true could help make this easier to read -> it must be true for showInheritedStyles to be true

::: toolkit/devtools/styleinspector/css-logic.js
@@ +41,5 @@
>  const { Cc, Ci, Cu } = require("chrome");
>  const Services = require("Services");
>  const DevToolsUtils = require("devtools/toolkit/DevToolsUtils");
>  
> +const RX_UNIVERSAL_SELECTOR = /\s*\*\s*/g;

Looks like some unrelated regexes have been re-added in this patch.  They should be safe to remove

@@ +50,5 @@
> +const RX_CLASS_OR_ATTRIBUTE = /\s*(?:\.\w+|\[.+?\])\s*/g;
> +const RX_PSEUDO = /\s*:?:([\w-]+)(\(?\)?)\s*/g;
> +
> +let pseudos = new Set([
> +  "after",

I'm thinking we just put the : or :: for each element here in this set instead of managing it in styles.js.  For ones that used to be there before and worked with ':' just keep it that way

(":first-line", ":first-letter", ":before", ":after", ":-moz-selection")

For all the new ones use '::' since that seems to be required

Comment 14

[Jen Fong-Adwent [:ednapiranha]]

Attachment: Bug1163183.patch

Finally! Turns out that I had to sort the entries order to match the elementStyle order.

Also had to leave `inherited: inherited` instead of `true` in _getAllElementRules since that is an object that carries information tested in test_styles-applied.html

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1a105695203b

Comment 15

[Brian Grinstead [:bgrins]]

Comment on attachment 8616258 [details] [diff] [review]
Bug1163183.patch

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

LGTM!

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/080dc5cf2910

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/080dc5cf2910

Comment 18

[Will Bamberg [:wbamberg]]

I've added something about this: https://developer.mozilla.org/en-US/docs/Tools/Page_Inspector/How_to/Examine_and_edit_CSS#Displaying_pseudo-elements.

Please let me know if it's all right.

Comment 19

[Jen Fong-Adwent [:ednapiranha]]

Hey Will, looks good except I see a duplicate entry of -moz-selection (I believe one should just be -selection?). The first being the fifth one listed and the second being the last one listed.

Comment 20

[Will Bamberg [:wbamberg]]

Oh, well spotted, thanks. I've made that correction.

Comment 21

[Nazir Ahmed Sabbir [:NaSb]]

Successfully reproduce the bug on 39.0a2 (2015-05-08) (Build ID: 20150508004008)

This Bug is now verified as fixed on Latest Aurora 41.0a2 (2015-07-29) (Build ID: 20150729004002)

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:41.0) Gecko/20100101 Firefox/41.0

Comment 22

[Forhad Hossain]

I have reproduced this bug with Firefox Aurora 39.0a2 (2015-05-08) (Build ID: 20150508004008) on Windows 64 bit.

Verified as fixed with Latest Aurora 41.0a2 (2015-08-05)(Build ID: 20150805004014)

Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:41.0) Gecko/20100101 Firefox/41.0

[bugday-20150805]

Comment 23

[Brian Grinstead [:bgrins]]

(In reply to Forhad Hossain from comment #22)
> I have reproduced this bug with Firefox Aurora 39.0a2 (2015-05-08) (Build
> ID: 20150508004008) on Windows 64 bit.
> 
> Verified as fixed with Latest Aurora 41.0a2 (2015-08-05)(Build ID:
> 20150805004014)
> 
> Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:41.0) Gecko/20100101
> Firefox/41.0
> 
> [bugday-20150805]

Thanks!