Closed Bug 915910 Opened 7 years ago Closed 7 years ago

[markup view] UI updates following refactor in Bug 855523

Categories

(DevTools :: Inspector, defect)

defect
Not set
normal

Tracking

(firefox26 verified, firefox27 verified)

VERIFIED FIXED
Firefox 27
Tracking Status
firefox26 --- verified
firefox27 --- verified

People

(Reporter: bgrins, Assigned: bgrins)

Details

Attachments

(2 files)

* 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.
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"
Assignee: nobody → pbrosset
> * 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;
 }
(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.
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;
 }
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.
Attachment #804954 - Flags: review?(mratcliffe)
Attachment #804954 - Flags: feedback?(pbrosset)
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.
Attachment #804954 - Flags: feedback?(pbrosset) → feedback+
(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.
Attachment #804954 - Flags: review?(mratcliffe) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/c32076505643
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 27
Assignee: pbrosset → bgrinstead
Target Milestone: Firefox 27 → ---
Target Milestone: --- → Firefox 27
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:
Attachment #804954 - Flags: approval-mozilla-aurora?
Attachment #804954 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Keywords: verifyme
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).
Status: RESOLVED → VERIFIED
Keywords: verifyme
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.