855523 - Nodes in the markup panel and rule/computed views are hard to expand via click

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[Victor Porof [:vporof][:vp]]

The clickable area defined by the twisty is a bit too small IMHO and very easy to miss. I also think that clicking the property names in the computed view should expand the corresponding item.

These are all just opinions :)

Comment 1

[Victor Porof [:vporof][:vp]]

I think that Chrome's approach here is wonderful and worth considering: when a line is hovered in the markup panel, then the entire line is highlighted and clickable.

Comment 2

[Patrick Brosset <:pbro>]

I've started working on a rework of the HTML structure behind the inspector that should support highlights and clicks on whole lines.

Comment 3

[Patrick Brosset <:pbro>]

Should be ready to submit a patch quite soon.
The rework of the HTML structure behind the markup inspector will allow 2 things:
- rows can be dblclicked anywhere to expand/collapse
- rows can be highlighted temporarily when a markup mutation occurs

Comment 4

[Patrick Brosset <:pbro>]

Pushed to try https://tbpl.mozilla.org/?tree=Try&rev=6eae81406ac7
As for the temporary highlight, this is more complex than anticipated, I've filed a separate bug for this: bug 911982

Comment 6

[Patrick Brosset <:pbro>]

Attachment: 855523-inspector-dbl-click.patch

Comment 7

[Patrick Brosset <:pbro>]

Try server reported no problems (only an unrelated intermittent failure of a profiler test, already reported in bug 908683).

Comment 8

[Patrick Brosset <:pbro>]

Attachment: 855523-inspector-dbl-click.patch

Changed the commit message

Comment 9

[Patrick Brosset <:pbro>]

Attachment: This is a temporary patch with the 3 files merged back together, to ease the review

Comment 10

[Patrick Brosset <:pbro>]

Attachment: This is a temporary patch with the 3 files merged back together, to ease the review

Comment 11

[Brian Grinstead [:bgrins]]

Comment on attachment 799309 [details] [diff] [review]
This is a temporary patch with the 3 files merged back together, to ease the review

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

This looks and feels a lot better than before.
* Lines should turn "active" color directly after clicking, not after clicking and hovering away.
* A weird thing I bumped into -> inspect a div on the following page, press Enter to start editing tag name, press Enter again.  Notice that the selection jumps back up to the body element: http://fiddle.jshell.net/bgrins/Zpfcc/show/
* Double clicking an empty tag (like script or link) will expand / collapse it even when there is no twisty arrow available.  Also happens with keyboard shortcuts, but I don't think it should be happening.  Opened Bug 912372 to deal with the keyboard shortcuts, but if a change happens in expandNode or _expandContainer it may resolve that but as well.
* Regarding the padding / font size trick to indent nodes - we discussed using a similar technique using a placeholder element with a width instead of padding.  If this works without complicating things it may be better since it would allow max indentation.

::: browser/devtools/markupview/markup-view.css
@@ +24,5 @@
> +  each level */
> +  font-size: 1.001em;
> +}
> +.tag-line {
> +  /* The right padding here is in em and hence is based on the em font-size

s/right/left

::: browser/devtools/markupview/markup-view.js
@@ +843,5 @@
>    this.elt.container = this;
>    this.children.container = this;
>  
> +  // Expanding/collapsing the node on dblclick of the whole tag-line element
> +  this._boundTagLineDblCkick = this._onTagLineDblClick.bind(this);

s/boundTagLineDblCkick/boundTagLineDblClick, or potentially this._onTagLineDblClick = this._onTagLineDblClick.bind(this); to prevent making new properties for this.

Comment 12

[Patrick Brosset <:pbro>]

Thanks Brian for that feedback.

> * Lines should turn "active" color directly after clicking, not after clicking and hovering away.

Done

> * A weird thing I bumped into -> inspect a div on the following page, press Enter to start editing tag name, press Enter again.  Notice that the selection jumps back up to the body element: http://fiddle.jshell.net/bgrins/Zpfcc/show/

Fixed

> * Double clicking an empty tag (like script or link) will expand / collapse it even when there is no twisty arrow available.  Also happens with keyboard shortcuts, but I don't think it should be happening.  Opened Bug 912372 to deal with the keyboard shortcuts, but if a change happens in expandNode or _expandContainer it may resolve that but as well.

Fixed too, so 912372 will need to be closed.

> * Regarding the padding / font size trick to indent nodes - we discussed using a similar technique using a placeholder element with a width instead of padding.  If this works without complicating things it may be better since it would allow max indentation.

I've tried with the extra element but it's not that simple because that element would need to be 100% of the height and floated. Furthermore, I think we can deal with cases where the inspector is resized really small by having a min-width on the root container and therefore we can keep the same indentation for all nodes.
I know the indentation logic is the weirdest part of the code, but the only other alternative I see would be to inject a style.padding on each tag line, via javascript, that would be calculated depending on the depth of the node. I reckon the CSS-only solution is more elegant.

> ::: browser/devtools/markupview/markup-view.css
> @@ +24,5 @@
> > +  each level */
> > +  font-size: 1.001em;
> > +}
> > +.tag-line {
> > +  /* The right padding here is in em and hence is based on the em font-size
> 
> s/right/left

Done

> ::: browser/devtools/markupview/markup-view.js
> @@ +843,5 @@
> >    this.elt.container = this;
> >    this.children.container = this;
> >  
> > +  // Expanding/collapsing the node on dblclick of the whole tag-line element
> > +  this._boundTagLineDblCkick = this._onTagLineDblClick.bind(this);
> 
> s/boundTagLineDblCkick/boundTagLineDblClick, or potentially
> this._onTagLineDblClick = this._onTagLineDblClick.bind(this); to prevent
> making new properties for this.

Done.

I will upload a new patch for this.

Comment 13

[Patrick Brosset <:pbro>]

Attachment: 855523-inspector-dbl-click.patch

Rebased against latest fx-team changes + implemented corrections as per :bgrins' feedback

Comment 14

[Patrick Brosset <:pbro>]

Attachment: 855523-inspector-dbl-click-MERGED.patch - To ease review, files are back as one

Comment 15

[Patrick Brosset <:pbro>]

Attachment: 855523-inspector-dbl-click-MERGED.patch - To ease review, files are back as one

Comment 16

[Paul Rouget [:paul]]

Comment on attachment 799379 [details] [diff] [review]
855523-inspector-dbl-click-MERGED.patch - To ease review, files are back as one

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

::: browser/devtools/markupview/markup-view.css
@@ +5,5 @@
> +/* Need this wrapper to contain our negative margin */
> +#root-wrapper {
> +  overflow: hidden;
> +  min-width: 250px;
> +}

This is what we call a "magic number".
Why 250px?

@@ +10,5 @@
> +/* Because of the way we deal with indentation */
> +#root {
> +  margin-left: -1000em;
> +}
> +

Make sure all the rules are separated by a \n.

@@ +28,5 @@
> +.tag-line {
> +  /* The left padding here is in em and hence is based on the em font-size
> +  of the current child */
> +  padding-left: 1000em;
> +  cursor: pointer;

This is very hacky :)

What about adding the level in JS (setAttribute("level", 42)), and then have:

tag-line {padding-left: 10ch}
tag-line[level=0] {padding-left: 0}
tag-line[level=1] {padding-left: 1ch}
tag-line[level=2] {padding-left: 2ch}
tag-line[level=3] {padding-left: 3ch}
...
tag-line[level=10] {padding-left: 10ch)}

It won't indent after level=10, and the tree will become flat, but it sounds good enough to me.

::: browser/devtools/markupview/markup-view.js
@@ +850,1 @@
>  

Do you remove these listeners at some point? (not sure it's needed though)

::: browser/themes/shared/devtools/dark-theme.css
@@ +52,2 @@
>  }
>  

This is used in the computed view. Does it still look good there?

::: browser/themes/shared/devtools/light-theme.css
@@ +52,2 @@
>  }
>  

Used in the computed view.

Comment 17

[Patrick Brosset <:pbro>]

Thanks for the review.

> ::: browser/devtools/markupview/markup-view.css
> @@ +5,5 @@
> > +/* Need this wrapper to contain our negative margin */
> > +#root-wrapper {
> > +  overflow: hidden;
> > +  min-width: 250px;
> > +}
> 
> This is what we call a "magic number".
> Why 250px?

Yes I know, but that's the best I could find to ensure the inspector can still be easily used even when resized to a really small panel.
Basically, the inspector has no horizontal scrollbar by default, and this is fine. Most nodes will be displayed on one line, except for a few nodes that may wrap on 2 or more lines because they have many attributes, or because they are deeply nested (so closer to the right edge of the panel). I think in 99% of the situation you don't want a horizontal scrollbar so this way you see all the markup without having to scroll. 
This number will make a scrollbar appear if you resize your window down, because at some stage, having no horizontal scrollbar is not such a good idea anymore.
What would be a better way to deal with this?

> @@ +10,5 @@
> > +/* Because of the way we deal with indentation */
> > +#root {
> > +  margin-left: -1000em;
> > +}
> > +
> 
> Make sure all the rules are separated by a \n.

Got it, thanks!

> @@ +28,5 @@
> > +.tag-line {
> > +  /* The left padding here is in em and hence is based on the em font-size
> > +  of the current child */
> > +  padding-left: 1000em;
> > +  cursor: pointer;
> 
> This is very hacky :)
> 
> What about adding the level in JS (setAttribute("level", 42)), and then have:
> 
> tag-line {padding-left: 10ch}
> tag-line[level=0] {padding-left: 0}
> tag-line[level=1] {padding-left: 1ch}
> tag-line[level=2] {padding-left: 2ch}
> tag-line[level=3] {padding-left: 3ch}
> ...
> tag-line[level=10] {padding-left: 10ch)}
> 
> It won't indent after level=10, and the tree will become flat, but it sounds
> good enough to me.

I understand this part can be weird, but we've been discussing a lot about this vs. doing it in javascript as you mention.
The advantages of this solution is that:
- it's css-only, so it doesn't require any js pos-processing of the markup
- on the js side, we don't always know how deep a node is, so I'm not really sure there is a way to set these attributes as you suggested. It may be that there is a mutation somewhere and a part of the tree only needs updating. The script would have to recursively check if it has a parent to figure out its nesting level.

> ::: browser/devtools/markupview/markup-view.js
> @@ +850,1 @@
> >  
> 
> Do you remove these listeners at some point? (not sure it's needed though)

No I don't. I originally wanted to but realized there were listeners already that weren't removed. :miker mentioned that the markup-view being in an iframe means removing the listeners isn't really necessary.

> ::: browser/themes/shared/devtools/dark-theme.css
> @@ +52,2 @@
> >  }
> >  
> 
> This is used in the computed view. Does it still look good there?
> 
> ::: browser/themes/shared/devtools/light-theme.css
> @@ +52,2 @@
> >  }
> >  
> 
> Used in the computed view.

I checked and can say that it looks better to me. It used to almost look the same as the background, now the "theme-bg-darker" actually looks darker than the background, giving more contrast.

Comment 18

[Patrick Brosset <:pbro>]

Attachment: 855523-inspector-dbl-click-MERGED.patch - To ease review, files are back as one

Comment 19

[Patrick Brosset <:pbro>]

For information, :bgrins and I came up with a better way of dealing with the indentation. Rather than using the weird font-size em increments, we now use a large negative margin combined with an even larger positive padding.
All other points raised in Brian's feedback and Paul's review have been fixed.

Comment 20

[Patrick Brosset <:pbro>]

Attachment: 855523-inspector-dbl-click-MERGED.patch - To ease review, files are back as one

Comment 21

[Patrick Brosset <:pbro>]

Thanks Mike.
Uploading a new patch now just to correct the commit message.

Comment 22

[Patrick Brosset <:pbro>]

Attachment: 855523-inspector-hard-to-click.patch

Final patch file, with correct commit message, after review granted by Mike.

Comment 23

[Brian Grinstead [:bgrins]]

Were you going to split the files up again in the final patch, or were you going to handle that in another checkin?  It looks like this is the merged patch still.

Comment 24

[Patrick Brosset <:pbro>]

Let's keep with this 1 big file. There's been changes to it since and it appears too dangerous to split it now.
I'll get this done after this patch lands.

Comment 25

[Paul Rouget [:paul]]

https://hg.mozilla.org/integration/fx-team/rev/1892b204f836

Comment 26

[Paul Rouget [:paul]]

backed out (orange) - https://hg.mozilla.org/integration/fx-team/rev/19c8f5a267fa

https://tbpl.mozilla.org/php/getParsedLog.php?id=27690827&tree=Fx-Team

Comment 27

[Patrick Brosset <:pbro>]

Will re-import my patch on the latest code level and launch the failing test locally.

Comment 28

[Patrick Brosset <:pbro>]

The issue is with the failing test case file itself in fact.

  <div id="1">
    #textnode
    <div id="2">

In the above case, the test, while selection was on #1, was pressing RIGHT to select the #textnode and then pressing RIGHT again would leave the selection on the same #textnode instead of going down to #2.
So it required 2 RIGHT key presses to move down to #2, but only the very first time. If you went up again and tried that once more, it would select #2.
With my patch, this "bug" was solved, hence the failing test.

Will attach a new patch soon.

Comment 29

[Patrick Brosset <:pbro>]

New TRY build ongoing https://tbpl.mozilla.org/?tree=Try&rev=8c14ea01c762

Comment 30

[Patrick Brosset <:pbro>]

Attachment: 855523-inspector-hard-to-click.patch

This patch fixes the failing test case.

Comment 31

[Patrick Brosset <:pbro>]

TRY build is green.
Review done.
Failing test case fixed.

Ready do be checked-in!

Comment 32

[Ryan VanderMeulen [:RyanVM]]

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

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/edee3e148f47