Bug 151628 – Web Inspector: REGRESSION: "Duplicate Selector" context menu item doesn't work

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Blaze Burg]

This command seems to send an invalid command to the backend. Here's the relevant protocol traffic:

[native code]: CONSOLE LOG frontend: {"id":222,"method":"CSS.addRule","params":{"contextNodeId":7,"selector":"#ac-gn-menustate"}}
[native code]: CONSOLE LOG backend: {"error":{"code":-32602,"message":"Some arguments of method 'CSS.addRule' can't be processed","data":[{"code":-32602,"message":"Parameter 'styleSheetId' with type 'String' was not found."},{"code":-32602,"message":"Some arguments of method 'CSS.addRule' can't be processed"}]},"id":222}

The command did not ship in Safari 9, so it's probably new. I don't know if it ever worked. We should extract the underlying command and add a test for this, since nobody even knew it was broken. I only found out via manual testing for another context menu related change.

[Radar WebKit Bug Importer]

<rdar://problem/23673487>

[Radar WebKit Bug Importer]

<rdar://problem/23673489>

[Devin Rousso]

(In reply to comment #0)
> The command did not ship in Safari 9, so it's probably new.

Yeah I added this over the summer.

> I don't know if it ever worked.

I can definitely confirm that it did work when it was added.

> We should extract the underlying command and add a test for this, since nobody even knew it was broken.

When I implemented this it didn't use any new commands and simply created a new rule using the selector of the rule under the contextmenu.  I could be wrong, but I'm pretty sure the addRule command already has a test for it.

[Blaze Burg]

(In reply to comment #3)
> (In reply to comment #0)
> > We should extract the underlying command and add a test for this, since nobody even knew it was broken.
> 
> When I implemented this it didn't use any new commands and simply created a
> new rule using the selector of the rule under the contextmenu.  I could be
> wrong, but I'm pretty sure the addRule command already has a test for it.

What I meant was that we should expose the same action that the context menu does, as part of a model or controller so that we can "click" it using a test. The failure is probably specific to how the backend command is being generated in the context menu code.

[Devin Rousso]

Created attachment 266210 [details]
Patch

So I think that the reason for the errors was that the "addRuleWithSelector" function inside DOMNodeStyles didn't have any of the fallbacks present in "addEmptyRule" (which is why "addEmptyRule" worked just fine even though it used the same CSSAgent calls).  I merged the two functions into one because I think it's silly to have two if they do practically the same thing.

[Blaze Burg]

Comment on attachment 266210 [details]
Patch

r=me, thanks. Should be covered by existing tests.

[WebKit Commit Bot]

Comment on attachment 266210 [details]
Patch

Clearing flags on attachment: 266210

Committed r192784: <http://trac.webkit.org/changeset/192784>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.