915910 - [markup view] UI updates following refactor in Bug 855523

Status: VERIFIED FIXED

(DevTools :: Inspector, defect)

Description

[Brian Grinstead [:bgrins]]

Attachment: Overflow on long attribute name

* New attribute node is not clickable anymore (click on the right side of a tag name and it should expand to allow easy adding of a new attribute).  Tabbing into it still works.
* Double clicking while an attribute is being edited should not expand/collapse.
* When adding a long attribute, the background highlight does not expand all the way to the right of the markup container, and it is tricky to scroll back to the left to get to the tree.

Comment 1

[Patrick Brosset <:pbro>]

Another remark from Brian via IRC:

"Hey, that issue with overflow on the markup view also happens when you have a field that expands when editing.. like if you had id="Lorem IpsumLorem IpsumLorem IpsumLorem IpsumLorem IpsumLorem IpsumLorem IpsumLorem IpsumLorem IpsumLorem IpsumLorem IpsumLorem Ipsum" or something it will look fine (word wrapped) until you start editing"

Comment 2

[Patrick Brosset <:pbro>]

> * New attribute node is not clickable anymore (click on the right side of a
> tag name and it should expand to allow easy adding of a new attribute). 
> Tabbing into it still works.

Correct, I really didn't see this one coming. Unfortunately, no tests failed either.
I'm guessing a simple onclick listener is missing on the > character.

> * Double clicking while an attribute is being edited should not
> expand/collapse.

Correct. I recall that the inplace-editor widget has a way of stopping propagation (a boolean can be set at initialization).
So it should be fairly easy.

> * When adding a long attribute, the background highlight does not expand all
> the way to the right of the markup container, and it is tricky to scroll
> back to the left to get to the tree.

I think the way to fix this:
 .attr-value {
   word-wrap: break-word;
 }

Comment 3

[Patrick Brosset <:pbro>]

(In reply to Patrick Brosset from comment #2)
> > * New attribute node is not clickable anymore (click on the right side of a
> > tag name and it should expand to allow easy adding of a new attribute). 
> > Tabbing into it still works.
> 
> Correct, I really didn't see this one coming. Unfortunately, no tests failed
> either.
> I'm guessing a simple onclick listener is missing on the > character.

Actually, for this one, it still works but is really tricky. If you click just 1px left or right of the > character, it'll work.
I'm suspecting this is because now, we have a mousedown listener for the whole line container element, and we stopPropagation at the end of the handler.

Comment 4

[Patrick Brosset <:pbro>]

Summing up my findings so far.

> * New attribute node is not clickable anymore (click on the right side of a
> tag name and it should expand to allow easy adding of a new attribute). 
> Tabbing into it still works.

Can be fixed by adding a <span class="closing-bracket">...</span> around the &gt; and adding the following CSS selector:

 .closing-bracket {
   pointer-events: none;
 }

> * Double clicking while an attribute is being edited should not
> expand/collapse.

There was no dblclick handler on the whole line container before, that's why this did not occur before.
I have no good solution in mind right now, but perhaps adding an IF in the _onToggle handler to test whether the dblclick happened inside an inplace-editor? Or patching the inplace-editor itself to stop propagation in this case.

> * When adding a long attribute, the background highlight does not expand all
> the way to the right of the markup container, and it is tricky to scroll
> back to the left to get to the tree.

Can be fixed with:
 .attr-value {
   word-wrap: break-word;
 }

Comment 5

[Brian Grinstead [:bgrins]]

Attachment: markupview-915910.patch

This is implementing suggestions from Patrick for handing items 1 and 2.  For issue 3, I'm using a float with min-width to contain the long inline children that happen either from unwrapped text, or from long input fields.    There may be another technique to handle this type of layout problem, but this was the best I found that would preserve the background color for the children even when the panel was scrolled to the right.

The most important thing is that this patch allows the user to scroll back to the left so they don't get stuck on the right side due to a long attribute name or comment, or a deep expanded DOM tree.  This is a pretty big UX issue, so I am hoping we can get a patch for this checked in before the uplift.

Comment 6

[Brian Grinstead [:bgrins]]

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=78d240f37bcc

Comment 7

[Patrick Brosset <:pbro>]

Comment on attachment 804954 [details] [diff] [review]
markupview-915910.patch

Seems perfect for me.
On top of using the float/max-width technique, why not also using this:
 .attr-value {
   word-wrap: break-word;
 }
I think it would make the UI look a bit better if indeed there are long strings with no spaces in them.

Comment 8

[Brian Grinstead [:bgrins]]

(In reply to Patrick Brosset from comment #7)
> Comment on attachment 804954 [details] [diff] [review]
> markupview-915910.patch
> 
> Seems perfect for me.
> On top of using the float/max-width technique, why not also using this:
>  .attr-value {
>    word-wrap: break-word;
>  }
> I think it would make the UI look a bit better if indeed there are long
> strings with no spaces in them.

It does look better wrapped.  But to do that we need to set the width (not min-width) on #root, which means that when text does overflow, the highlighted background (and ability to hover the nodes when scrolled) does not expand beyond the initial size of the body - just like Attachment #804066 [details] .  This happens mostly during editing, but also with comments.  I'm sure we can find a work around for this, but I would like to spend some more time testing it out (so maybe something we could handle in another bug).  

If I'm missing an easy way to handle this, then it would be great to get the word wrapping in, though long string with no breaking characters are probably not *that* common as far as making a huge scrollbar.  It does look better in general and more consistent in general to break everywhere rather than at normal breaking locations, as right now you can get jagged line endings in the tag markup.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

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

Comment 10

[Ryan VanderMeulen [:RyanVM]]

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

Comment 11

[Brian Grinstead [:bgrins]]

Comment on attachment 804954 [details] [diff] [review]
markupview-915910.patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 855523
User impact if declined: User will not be able to scroll to left in inspector if content is wider than viewport.
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): Risk is limited to the frontend of markup tree view in inspector tab.  Specifically, changing the CSS layout could cause visual alignment issues. 
String or IDL/UUID changes made by this patch:

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e94c5d22c44e

Comment 13

[Bogdan Maris, Desktop QA]

Mozilla/5.0 (X11; Linux i686; rv:26.0) Gecko/20100101 Firefox/26.0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:26.0) Gecko/20100101 Firefox/26.0

Verified as fixed on latest Aurora (buildID: 20131003004003) and latest Nightly (buildID: 20131003030203).