Bug 44311 – Web Inspector: elements dom tree - word wrap toggle

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

[Adam]

I'd really like to see a toggle switch for turning word-wrap on/off on the DOM tree.  Depending on how deep the tree is, you need a really wide monitor to see everything without it being very cramped.  It would be good if it was an option so be able to turn it off and add a scroll bar.  Otherwise on a deep tree with word-wrap, it's 80% white space and a tall line of text.

It is convenient for shallow trees, it could be annoying to have to keep scrolling half an inch back and forth.  So it should really be optional.

[Joseph Pecoraro]

That may be nice. Note that you can re-root the tree at a given node.
Just double click the node in the breadcrumbs (bottom of the element's
panel). This should help some.

[Joseph Pecoraro]

Created attachment 64912 [details]
[IMAGE] Rooted at HTML

[Joseph Pecoraro]

Created attachment 64913 [details]
[IMAGE] Rooted at div#ten

[Adam]

Sweet, didn't realize that double clicking did that.

[Alexander Pavlov (apavlov)]

Created attachment 84861 [details]
[PATCH] Suggested solution

A "Word Wrap" context menu check item will switch between the word-wrapped and non-word-wrapped modes of the DOM display.

[Alexander Pavlov (apavlov)]

Created attachment 84862 [details]
[IMAGE] Screenshot of a non-word-wrapped DOM with a context menu displayed

[Pavel Feldman]

Comment on attachment 84861 [details]
[PATCH] Suggested solution

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

Did you check whether revealing element reveals it horizontally Ok?

> Source/WebCore/inspector/front-end/ElementsTreeOutline.js:196
> +        var x = Math.max(Math.min(root.totalOffsetLeft + root.offsetWidth, root.parentElement.totalOffsetLeft + root.parentElement.offsetWidth) - 20, 1);

Could you use wrap / nowrap conditions here and simplify the code? r- for this.

[Alexander Pavlov (apavlov)]

(In reply to comment #7)
> (From update of attachment 84861 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=84861&action=review
> 
> Did you check whether revealing element reveals it horizontally Ok?
Good point, I'm pretty sure it only scrolls to the necessary line.

> > Source/WebCore/inspector/front-end/ElementsTreeOutline.js:196
> > +        var x = Math.max(Math.min(root.totalOffsetLeft + root.offsetWidth, root.parentElement.totalOffsetLeft + root.parentElement.offsetWidth) - 20, 1);
> 
> Could you use wrap / nowrap conditions here and simplify the code? r- for this.
This is not about wrap/nowrap. This code constrains the x coordinate to the visible portion of the corresponding list item. I don't see a point in having two similar conditions instead of one -- it clutters the code (especially given that this code is wrap-agnostic at the moment).

[Pavel Feldman]

Comment on attachment 84861 [details]
[PATCH] Suggested solution

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

>>> Source/WebCore/inspector/front-end/ElementsTreeOutline.js:196
>>> +        var x = Math.max(Math.min(root.totalOffsetLeft + root.offsetWidth, root.parentElement.totalOffsetLeft + root.parentElement.offsetWidth) - 20, 1);
>> 
>> Could you use wrap / nowrap conditions here and simplify the code? r- for this.
> 
> This is not about wrap/nowrap. This code constrains the x coordinate to the visible portion of the corresponding list item. I don't see a point in having two similar conditions instead of one -- it clutters the code (especially given that this code is wrap-agnostic at the moment).

Two things concern me:
1) Math.min(root.totalOffsetLeft + root.offsetWidth, root.parentElement.totalOffsetLeft + root.parentElement.offsetWidth)
    So you imply that container can be smaller that the child. Is this related to the scroll? If yes, why not to check explicitly
2) Math.max(, 1) implies we now hit negative numbers

Formula this big is 100 times worse that 5 lines of clean conditional code. It is hard to understand, it is hard to maintain, it is hard not to regress.

[Alexander Pavlov (apavlov)]

Created attachment 85161 [details]
[PATCH] Comments addressed, added revealing of the element's left boundary (if necessary)

[Pavel Feldman]

Comment on attachment 85161 [details]
[PATCH] Comments addressed, added revealing of the element's left boundary (if necessary)

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

> Source/WebCore/inspector/front-end/ElementsTreeOutline.js:188
> +        var container = this.element.parentElement;

What is wrong with the native implementation? We should use platform capabilities where possible.

> Source/WebCore/inspector/front-end/ElementsTreeOutline.js:201
> +        // items extend nearly to the right edge of the outer <ol>. However,

You could do:
var scrollContainerElement = this.element.parentElement;
and then leave old code changed to using scrollContainerElement.

[Alexander Pavlov (apavlov)]

Created attachment 85168 [details]
[PATCH] Comments addressed

[Alexander Pavlov (apavlov)]

Committing to http://svn.webkit.org/repository/webkit/trunk ...
        M       Source/WebCore/ChangeLog
        M       Source/WebCore/English.lproj/localizedStrings.js
        M       Source/WebCore/inspector/front-end/ElementsPanel.js
        M       Source/WebCore/inspector/front-end/ElementsTreeOutline.js
        M       Source/WebCore/inspector/front-end/Settings.js
        M       Source/WebCore/inspector/front-end/inspector.css
Committed r80708