Bug 110641 – WebInspector: Switch hide element shortcut in ElementsPanel to use a selector

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

WebInspector: Switch hide element shortcut in ElementsPanel to use a selector

Created attachment 189824 [details]
Patch

When the shortcut is used now, the class inspector-hide-shortcut is added to the element's class attribute and the rule:

.inspector-hide-shortcut, .inspector-hide-shortcut * {
    visibility: hidden !important;
}

is added to the temporary inspector stylesheet. This way all of the child elements get hidden as well.

https://code.google.com/p/chromium/issues/detail?id=176665

[Pavel Feldman]

Comment on attachment 189824 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=189824&action=review

> Source/WebCore/inspector/front-end/ElementsTreeOutline.js:491
> +                this._addHideShortcutRuleToInspectorStyleSheet(node.id, hideShortcutSelector);

You only need to do this once.

> Source/WebCore/inspector/front-end/ElementsTreeOutline.js:634
> +            if (!resource)

Resource may exist due to user's add rule action.

> Source/WebCore/inspector/front-end/ElementsTreeOutline.js:638
> +        WebInspector.cssModel.getViaInspectorResourceForRule(this._hideRule || {}, resourceCallback.bind(this));

We need to move this into model via creating addInspectorRule method that receives both selector and rule body.

Created attachment 190161 [details]
Patch

Comment on attachment 189824 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=189824&action=review

I tried to work on your comments, but I ended up with almost the same as before. It already works exactly the way we want, but because of how the methods for accessing the inspector resource are implemented on CSSStyleModel, this looks a bit weird. For example I can't just ask if the inspector resource exists, I can only check if a certain rule is associated with the resource (CSSStyleModel::getViaInspectorResourceForRule), also I can't just create the resource, I have to use CSSStyleModel::addRule which then creates the resource in the backend.

The new patch encapsulates the functionality of the hide shortcut a bit better and has some comments explaining why it works.

>> Source/WebCore/inspector/front-end/ElementsTreeOutline.js:491
>> +                this._addHideShortcutRuleToInspectorStyleSheet(node.id, hideShortcutSelector);
> 
> You only need to do this once.

This method is called more than once, but the style rule is only once added to the inspector resource.

>> Source/WebCore/inspector/front-end/ElementsTreeOutline.js:634
>> +            if (!resource)
> 
> Resource may exist due to user's add rule action.

Yes, but if this._hideRule was not set yet, then no resource is found. I found no better way of checking whether the resource has the rule so far.

>> Source/WebCore/inspector/front-end/ElementsTreeOutline.js:638
>> +        WebInspector.cssModel.getViaInspectorResourceForRule(this._hideRule || {}, resourceCallback.bind(this));
> 
> We need to move this into model via creating addInspectorRule method that receives both selector and rule body.

CSSStyleModel.addRule() already adds the rule to the inspector stylesheet. A method that also takes the rule body as argument would look something like this:

addInspectorRule: function(nodeID, selector, name, value)
{
    function successCallback(rule)
    {
        rule.style.appendProperty(name, value);
    }
    function failureCallback() {}

    this.addRule(nodeID, selector, successCallback, failureCallback);
}

[Pavel Feldman]

Comment on attachment 190161 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=190161&action=review

> Source/WebCore/inspector/front-end/ElementsTreeOutline.js:625
> +                this._hideShortcutRule = newRule;

There is an inspector stylesheet per document, you can't store them all in a single variable.

> Source/WebCore/inspector/front-end/ElementsTreeOutline.js:641
> +                WebInspector.cssModel.addRule(node.id, hideShortcutSelector, successCallback.bind(this), failureCallback);

I am still not convinced this will only run once per inspector stylesheet. Here is the scenario for you:
1) On a page, press "+" in the styles sidebar to add user-defined style rule.
2) Run _toggleHideShortcut

I suspect that resource will be defined and hence you will not add the new rule.

We might want a test for this.

Comment on attachment 190161 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=190161&action=review

>> Source/WebCore/inspector/front-end/ElementsTreeOutline.js:625
>> +                this._hideShortcutRule = newRule;
> 
> There is an inspector stylesheet per document, you can't store them all in a single variable.

this._hideShortcutRule is only set once per document. When the document changes, the inspector stylesheet is removed and CSSStyleModel::getViaInspectorResourceForRule(this._hideShortcutRule) does not return a valid resource, so a new rule is added, which forces the inspector stylesheet to be created.

>> Source/WebCore/inspector/front-end/ElementsTreeOutline.js:641
>> +                WebInspector.cssModel.addRule(node.id, hideShortcutSelector, successCallback.bind(this), failureCallback);
> 
> I am still not convinced this will only run once per inspector stylesheet. Here is the scenario for you:
> 1) On a page, press "+" in the styles sidebar to add user-defined style rule.
> 2) Run _toggleHideShortcut
> 
> I suspect that resource will be defined and hence you will not add the new rule.
> 
> We might want a test for this.

I tested this scenario multiple times now and it works. "+" creates the inspector stylesheet, but this._hideShortcutRule is still undefined so CSSStyleModel::getViaInspectorResourceForRule(null) returns no resource.

I'm not arguing that this is a good solution, if you can point me another direction I'm happy to do so. This is the best way I could find.

I will have a look into writing a test.

Created attachment 190412 [details]
Patch

I added a test case for the hide shortcut. I have never written tests before, so please give me some feedback.

It does not cover your proposed scenario though, because I couldn't figure out how to remove the inspector style-sheet again once it was added.

[Pavel Feldman]

It sounds like we need a better way of adding this rule than inspector stylesheet. In the inspector-stylesheet user will be able to delete it.

[Alexander Pavlov (apavlov)]

(In reply to comment #10)
> It sounds like we need a better way of adding this rule than inspector stylesheet. In the inspector-stylesheet user will be able to delete it.

At the moment, via-inspector stylesheet rules are not deletable (in fact no rules are deletable), but this feature implementation is planned. So, for you to be on the safe side, I suggest injecting a separate inline stylesheet using Runtime.evaluate() on all page frames.

Created attachment 190613 [details]
Patch

(In reply to comment #11)
> (In reply to comment #10)
> > It sounds like we need a better way of adding this rule than inspector stylesheet. In the inspector-stylesheet user will be able to delete it.
> 
> At the moment, via-inspector stylesheet rules are not deletable (in fact no rules are deletable), but this feature implementation is planned. So, for you to be on the safe side, I suggest injecting a separate inline stylesheet using Runtime.evaluate() on all page frames.

I followed your suggestion and inject the style now via Runtime.evaluate().

[Alexander Pavlov (apavlov)]

Comment on attachment 190613 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=190613&action=review

> Source/WebCore/ChangeLog:11
> +        the document by creating a style tag in the documents head and in the head of each

Could you test that the feature works for nested frames, too? Please see LayoutTests/inspector/styles/resources/styles-source-lines-inline-iframe.html for how to synchronize on iframe loading.

> Source/WebCore/inspector/front-end/DOMAgent.js:800
> +        var classString = this.getAttribute("class") || "";

You could implement this using the same RuntimeAgent.evaluate() approach as below if you could retrieve the target node in the evaluated code. Pavel can advise on this. If that were possible, the code would be as simple as

var toggleClass = function(className, enable)
{
    var node = ...
    if (enable)
        node.classList.add(className);
    else
        node.classList.remove(className);
}

> Source/WebCore/inspector/front-end/ElementsTreeOutline.js:620
> +     * @param {function()=} userCallback

This signature should match what toggleClassName() accepts:

{function(?Protocol.Error)=}

> Source/WebCore/inspector/front-end/ElementsTreeOutline.js:624
> +        var className = "web-inspector-hide-shortcut";

I would use some weird system-like prefix for these className and styleTagId, perhaps "__" or similar, in order to avoid collisions with the user code as much as possible.

> Source/WebCore/inspector/front-end/ElementsTreeOutline.js:628
> +        var injectStyleRuleCode = [

You can just declare "var injectStyleRuleCode = function() {...}" and then evaluate something like
"(" + injectStyleRuleCode.toString() + ")()"
(take a look at how a similar thing is done in DOMAgent.prototype._emulateTouchEventsChanged. That approach will barely work for you, since it requires a page reload for the code to get injected).

> Source/WebCore/inspector/front-end/ElementsTreeOutline.js:629
> +            "var style = document.head.querySelector('style#" + styleTagId + "');",

Excellent

> Source/WebCore/inspector/front-end/ElementsTreeOutline.js:646
> +        RuntimeAgent.evaluate(injectStyleRuleCode, runtimeCallback.bind(this));

Are you sure it evaluates the script in every frame out there?

> LayoutTests/inspector/elements/hide-shortcut.html:137
> +Tests the hide shortcut, which toggles a class name on the node and sets visibility: hidden on the node and it's ancestors.

We usually specify a link to the respective webkit bug after the description:

...and its ancestors. <a href="https://bugs.webkit.org/show_bug.cgi?id=110641">Bug 110641</a>.

Created attachment 190782 [details]
Patch

I put the class toggling and the style rule injection in one script, which is called on the node's remote object. This way the style rule is always added to the document where the node is in, so this also works within frames.

[WebKit Review Bot]

Comment on attachment 190782 [details]
Patch

Clearing flags on attachment: 190782

Committed r144405: <http://trac.webkit.org/changeset/144405>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.