672003 - [highlighter] Toggleable classes in the selected node's "infobar"

Status: RESOLVED WONTFIX

(DevTools :: General, defect, P3)

Description

[Rob Campbell [:rc] (:robcee)]

The classlist in the selected node's "infobar" (toolbar) should be toggleable. That is, when clicking one it removes it from the node's classlist. Clicking it again re-adds it. It will provide a quick way to see the effects of a given class on a node in the highlighter

Comment 1

[Rob Campbell [:rc] (:robcee)]

this bug needs a patch, badly!

Comment 2

[Dave Camp (:dcamp)]

I'm assuming, in that case, that [has-patch] was in error.

Comment 3

[Dave Camp (:dcamp)]

Oh, it was a hoarded patch.

Comment 4

[Paul Rouget [:paul]]

Attachment: patch v0.1 WIP

Comment 5

[Paul Rouget [:paul]]

Attachment: patch v0.2 WIP

rebased.

Comment 6

[Paul Rouget [:paul]]

Attachment: patch v0.3 (no test)

The highlighter is not updated any more when class change. The mutation events should trigger an update (bug 566085).

Comment 7

[Paul Rouget [:paul]]

Attachment: patch v1

Comment 8

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 560599 [details] [diff] [review]
patch v1

in inspector.js

nit!
+   */
+   buildInfobarClassButton:
+     function Highlighter_buildInfobarClassButton(aClass, aDisabled)

function indentation should be under the b in buildInfo...

and,
+  getAlteredClassList:  function Highlighter_getAlteredClassList(aNode)

extra space after the :

I can't see what method the if (alteredClasses) block is in because I don't have context. Is that for loop being executed everytime the highlighter is repositioned? It could be costly.

+    aNode.classList.add(aClass);
+  },
+
+
+
+
   /////////////////////////////////////////////////////////////////////////
   //// Event Handling

extra gap between the end of this method and the event handling section.

I think this is pretty decent. Still need to see it in action to get a feel for the behavior.

Comment 9

[Paul Rouget [:paul]]

Attachment: patch v1.1 - rebased

Rebased (from inspector.js to the jsm) + rob's comments.



(In reply to Rob Campbell [:rc] (robcee) from comment #8)
> Comment on attachment 560599 [details] [diff] [review] [diff] [details] [review]
> patch v1
>
> I can't see what method the if (alteredClasses) block is in because I don't
> have context. Is that for loop being executed everytime the highlighter is
> repositioned? It could be costly.

It is in this loop (updateInfobar()), and it is updated when a new node is selected (not when the highlighter is repositioned). Yes, it is costly, but not more than the original updateInfobar. What do you consider costly here?

Comment 10

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 562742 [details] [diff] [review]
patch v1.1 - rebased

in inspector.jsm:

this:
+    let alteredClasses = this.getAlteredClassList(this.node);
+
+    if (alteredClasses) {
       let fragment = this.chromeDoc.createDocumentFragment();
-      for (let i = 0; i < this.node.classList.length; i++) {
-        let classLabel = this.chromeDoc.createElement("label");
-        classLabel.className = "highlighter-nodeinfobar-class plain";
-        classLabel.textContent = this.node.classList[i];
-        fragment.appendChild(classLabel);
+      for (let oneClass in alteredClasses) {
+        let disabled = alteredClasses[oneClass];
+        let button = this.buildInfobarClassButton(oneClass, disabled);
+        fragment.appendChild(button);
       }
       classes.appendChild(fragment);
+    } else {
+      if (this.node.className) {
+        let fragment = this.chromeDoc.createDocumentFragment();
+        for (let i = 0; i < this.node.classList.length; i++) {
+          let oneClass = this.node.classList[i];
+          let button = this.buildInfobarClassButton(oneClass, false);
+          fragment.appendChild(button);
+        }
+        classes.appendChild(fragment);
+      }

I think can be replaced with something like:

let alteredClasses = this.getAlteredClassList(this.node);
let fragment = this.chromeDoc.createDocumentFragment();

for (let i = 0; i < this.node.classList.length; i++) {
  let oneClass = this.node.classList[i];
  let disabled = alteredClasses && alteredClasses[oneClass];
  let button = this.buildInfobarClassButton(oneClass, disabled);
  fragment.appendChild(button);
}

classes.appendChild(fragment);

... though this didn't work as included here in testing for some reason.

+  buildInfobarClassButton:
+    function Highlighter_buildInfobarClassButton(aClass, aDisabled)
+  {

indent function directly under the property, e.g., 

buildInfobarClassButton:
function Highlighter...

NIT in getAlteredClassList:

+    for (let i = 0; i < this.alteredNodes.length; i++)
+    {

try to be consistent in your bracket cuddling. :)

this patch tests fine in both opt and debug, so that's good. I did make a couple of observations though:

1) The HTML panel isn't updated if shown when a class is removed. This could probably be done in a followup.

2) Same probably applies to Style Inspector.

3) Switching tabs and returning presents an incorrect view of the content page. Classes that were disabled are just gone and unable to be restored unless you reload.

I'm a little worried about modifying content like this. One of the goals of this tool was to not make any changes to content if at all possible, though modifying attributes is already possible. It just feels like toggling a class through this type of button shouldn't be quite so destructive.

Any ideas? Welcome feedback and opinions from others here.

Comment 11

[Dave Camp (:dcamp)]

(In reply to Rob Campbell [:rc] (robcee) from comment #10) 
> I'm a little worried about modifying content like this. One of the goals of
> this tool was to not make any changes to content if at all possible, though
> modifying attributes is already possible. It just feels like toggling a
> class through this type of button shouldn't be quite so destructive.
> 
> Any ideas? Welcome feedback and opinions from others here.

I understood the goal to be more like "Don't modify content as an implementation detail of the tool", not "don't allow modification of content through this tool".

Toggling the classes seems useful for quickly understanding their effects on rendering.  We could maybe do that non-destructively through some sort of shenanigans (somehow rendering with one set of classes but maintaining the dom unmodified) but that sounds more difficult to explain/understand than just modifying the dom.

We could consider ways to make the interaction more explicit (maybe just clicking is too easy for a destructive change).

If we're more concerned about the minor data loss than mutations, we could maintain a weakmap of nodes -> classes-that-have-been-toggled-off-with-this-ui, so that we don't lose the toggled-off classes after changing selected nodes.

Comment 12

[Dave Camp (:dcamp)]

(In reply to Rob Campbell [:rc] (robcee) from comment #10)

> 1) The HTML panel isn't updated if shown when a class is removed. This could
> probably be done in a followup.
> 
> 2) Same probably applies to Style Inspector.

Yeah, a convincing case could be made that we should wait to land (or enable) this until all the tools are properly responding to content changes.

Comment 13

[Rob Campbell [:rc] (:robcee)]

(In reply to Dave Camp (:dcamp) from comment #11)
> (In reply to Rob Campbell [:rc] (robcee) from comment #10) 
> > I'm a little worried about modifying content like this. One of the goals of
> > this tool was to not make any changes to content if at all possible, though
> > modifying attributes is already possible. It just feels like toggling a
> > class through this type of button shouldn't be quite so destructive.
> > 
> > Any ideas? Welcome feedback and opinions from others here.
> 
> I understood the goal to be more like "Don't modify content as an
> implementation detail of the tool", not "don't allow modification of content
> through this tool".
> 
> Toggling the classes seems useful for quickly understanding their effects on
> rendering.  We could maybe do that non-destructively through some sort of
> shenanigans (somehow rendering with one set of classes but maintaining the
> dom unmodified) but that sounds more difficult to explain/understand than
> just modifying the dom.
> 
> We could consider ways to make the interaction more explicit (maybe just
> clicking is too easy for a destructive change).
> 
> If we're more concerned about the minor data loss than mutations, we could
> maintain a weakmap of nodes ->
> classes-that-have-been-toggled-off-with-this-ui, so that we don't lose the
> toggled-off classes after changing selected nodes.

I think the dataloss is the bigger deal here. Mutation is acceptable, otherwise, how else are we going to be able to try out changes unless we load a separate copy of a web page for manipulating with the tools. I think it should be a goal that users can get back to the original state no matter what happens.

(In reply to Dave Camp (:dcamp) from comment #12)
> (In reply to Rob Campbell [:rc] (robcee) from comment #10)
> 
> > 1) The HTML panel isn't updated if shown when a class is removed. This could
> > probably be done in a followup.
> > 
> > 2) Same probably applies to Style Inspector.
> 
> Yeah, a convincing case could be made that we should wait to land (or
> enable) this until all the tools are properly responding to content changes.

Probably not a bad idea. Though we could also land this while working on the problem. That might be preferable to having to continually rebase this until we get there.

I think we could get most of what we need with a toolsUpdate(node) method in IUI that can be called whenever a mutation occurs.

Comment 14

[Paul Rouget [:paul]]

WONTFIX?

Comment 15

[Paul Rouget [:paul]]

If someone thinks we still need this feature, feel free to reopen.