[highlighter] [infobar] limit the size of the infobar

RESOLVED FIXED in Firefox 13

Status

()

Firefox
Developer Tools
P3
normal
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: paul, Assigned: bwinton)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 13
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug][mentor=paul][lang=css], URL)

Attachments

(1 attachment, 8 obsolete attachments)

(Reporter)

Description

6 years ago
Some websites use a lot of classes, and it makes the infobar way too large (larger than the window). We should limit the size of the infobar to 90% of the window.

Comment 1

6 years ago
We're doing developer tool prioritization, filter on 'brontozaur'
to ignore the spam.
Priority: -- → P3
(Reporter)

Updated

6 years ago
Whiteboard: [good first bug][mentor=paul][lang=css]
(Assignee)

Comment 2

6 years ago
So, what did you want to happen here?  I can see a couple of ways of handling this.

(I'm using the example document 'data:text/html;charset=UTF-8,<body><h1>Testing</h1><p id="testid" class="this is a test class with a ton of classes in it to see whether squeamish ossifrage is a good passphrase or not /Volumes/SSD/Programming/firefox/mozilla-central/layout/base/nsDocumentViewercpp VGhpcyBpcyBhIHRlc3QK">paragraph</p>'.)

1) collapse the middle stuff, to get "P#testid.this.is.a….VGhpcyBpcyBhIHRlc3QK".
2) show the whole thing, but on multiple lines, to get "P#testid.this.is.a.test.class.with.a.ton.of.classes.in.it.to.see.whether.squeamish.ossifrage.is.a.good.passphrase.or.not./Volumes/SSD/Programming/firefox/mozilla-central/layout
/base/nsDocumentViewercpp.VGhpcyBpcyBhIHRlc3QK"

(I've got a test patch up that does #2 by changing the highlighter-nodeinfobar-classes from an hbox containing labels to a description whose textContent we set, but the wrapping causes the height to not be what we think it should so things get mispositioned, and also causes reflow in undesirable ways when you click the "P" button.)

Thanks,
Blake.
(Reporter)

Comment 3

6 years ago
Hey, thank you for looking at this.

I believe we want 1) with crop=end, as I consider there are no reason to consider the end more important than any other part (and cropping in the middle could hide the id).
(Reporter)

Updated

6 years ago
(Assignee)

Comment 4

6 years ago
Created attachment 583020 [details] [diff] [review]
The first cut at the patch.

Okay, so, the only way I could find to get the overflow working in XUL was to set the width we wanted in pixels in Javascript.  Of course, this doesn't handle window resizes well, but I'm not sure how much that's wanted.  (It is, at least, not worse than the current behaviour…  ;)

(I also admit to not having run the tests due to not knowing how, but I _think_ I made the appropriate changes there…)

Thanks,
Blake.
Assignee: nobody → bwinton
Status: NEW → ASSIGNED
Attachment #583020 - Flags: review?(rcampbell)
(Reporter)

Comment 5

6 years ago
Comment on attachment 583020 [details] [diff] [review]
The first cut at the patch.

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

::: browser/devtools/highlighter/inspector.jsm
@@ +511,5 @@
> +      let fragment = "";
> +      for (let i = 0; i < this.node.classList.length; i++)
> +        fragment += "." + this.node.classList[i];
> +      classes.textContent = fragment;
> +

Since you use a single label, you should use setAttribute(value, fragment),
then you will be able to use the crop=end attribute instead of:
+  overflow-y: hidden;
+  text-overflow: ellipsis;

@@ +513,5 @@
> +        fragment += "." + this.node.classList[i];
> +      classes.textContent = fragment;
> +
> +      if (classes.clientWidth > this.win.innerWidth)
> +        classes.style.width = (this.win.innerWidth - baseWidth - 20) + "px";

Please don't use a magic number.

Also, do you really have to do this? Isn't there a way to do that with -moz-calc (I'm not sure about this)?
(Assignee)

Comment 6

6 years ago
(Added a better test url for an upcoming patch which gets us closer, I think.)
(Assignee)

Comment 7

6 years ago
Created attachment 583191 [details] [diff] [review]
The second cut at the patch.

Okay, this handles resizes a little better.

There are still some oddities, as you can see in http://dl.dropbox.com/u/2301433/Screenshots/DevTools/V2Overflows.png (which is results of this patch run on the current url).

To address Paul's suggestions from the previous patch, 1) using <label value="xxx"> doesn't let us select the styles, which isn't great.  I also couldn't get it to crop for some reason.  2) The magic number is gone, since we don't set the width in pixels anymore.  :)

Thanks,
Blake.
Attachment #583020 - Attachment is obsolete: true
Attachment #583020 - Flags: review?(rcampbell)
Attachment #583191 - Flags: feedback?(paul)
(Reporter)

Comment 8

6 years ago
Comment on attachment 583191 [details] [diff] [review]
The second cut at the patch.

>diff --git a/browser/base/content/highlighter.css b/browser/base/content/highlighter.css
> #highlighter-nodeinfobar {
>   display: block;
>   white-space: nowrap;
>   direction: ltr;
>+  max-width: 100%;
> }

Why is that useful?

>+.highlighter-nodeinfobar-class {
>+  overflow-y: hidden;
>+  text-overflow: ellipsis;
>+  -moz-box-flex: 1;
>+  max-width: 90%;
>+}

Why 90%? I think this explains the second part of your screenshot.

And yes, indeed, there are oddities. I believe we don't use the right tools here.
flex box model + <labels> + text-overflow: ellipsis don't look to work well together.
I have been playing with your code and got some weird results too.

I would try a totally different approach. Maybe using classic HTML without moz-box.
Or maybe with a <description>.
(Assignee)

Comment 9

6 years ago
Created attachment 583893 [details] [diff] [review]
The third, and hopefully penultimate, version of the patch.

(In reply to Paul Rouget [:paul] from comment #8)
> > #highlighter-nodeinfobar {
> >   display: block;
> >   white-space: nowrap;
> >   direction: ltr;
> >+  max-width: 100%;
> > }
> Why is that useful?

My understanding was that you needed a width on a parent element to set a percentage width on a child element (or it would be treated as "auto").  But that seems not to be the case, so I'll remove it.

> >+.highlighter-nodeinfobar-class {
> >+  overflow-y: hidden;
> >+  text-overflow: ellipsis;
> >+  -moz-box-flex: 1;
> >+  max-width: 90%;
> >+}
> Why 90%? I think this explains the second part of your screenshot.

So, I was hoping that this would make the class label be at most 90% of the infobar, but it looks like it might instead be making it 90% of its content.

> And yes, indeed, there are oddities. I believe we don't use the right tools
> here.
> flex box model + <labels> + text-overflow: ellipsis don't look to work well
> together.
> I have been playing with your code and got some weird results too.

Well, it's good to know I'm not missing something simple.  :)

> I would try a totally different approach. Maybe using classic HTML without
> moz-box.  Or maybe with a <description>.

Yeah, I think so too.  I'll take a stab at the classic HTML today (over lunch, perhaps).

Huh, I guess I forgot to submit this comment this morning.  Well, I took a stab, and here it is.  It's close.  The resizing behaves as I expect it to.  The last problem before I ask for official review is that when the arrow points down, it overlaps the selected white region.  (Any comments on things to fix in the code, or suggestions for how to fix the misaligned arrow are more than welcome!)

Thanks,
Blake.
Attachment #583191 - Attachment is obsolete: true
Attachment #583191 - Flags: feedback?(paul)
Attachment #583893 - Flags: feedback?(paul)
(Assignee)

Comment 10

6 years ago
Created attachment 583925 [details] [diff] [review]
The possibly but probably not final version of the patch.

So, the reason the bar was overlapping was because when we called container.getBoundingClientRect(), there was no content in the spans, so they collapsed, and we got the wrong height.  So now I'm adding some content to the classes span before that call, to give us the correct height.  This is safe, because the classes will get emptied out in the call to updateInfobar.

And with that fixed, I think this is good for review.  I don't know if you want ui-review, too, or whom to ask if you do.  (I'm also asking you, Paul, for review in honour of your recent peerage of the devtools module.  ;)

Thanks,
Blake.
Attachment #583893 - Attachment is obsolete: true
Attachment #583893 - Flags: feedback?(paul)
Attachment #583925 - Flags: review?(paul)
(Reporter)

Updated

6 years ago
Blocks: 708257
(Reporter)

Comment 11

6 years ago
Looks good to me. r=me once this test passes:

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/highlighter/test/browser_inspector_infobar.js | node 0: classes match. - Got .class1.class2, expected .class1 .class2

(spaces problem)
(Assignee)

Comment 12

6 years ago
Created attachment 588496 [details] [diff] [review]
The final version of the patch.

(r=paul, as per the previous comment.)

For reference, here's the final version of the patch, with the passing test.  :)

I'll commit it once I figure out the landing procedure for m-c.  ;)

Later,
Blake.
Attachment #583925 - Attachment is obsolete: true
Attachment #583925 - Flags: review?(paul)
Attachment #588496 - Flags: review+
(Reporter)

Comment 13

6 years ago
(In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #12)
> Created attachment 588496 [details] [diff] [review]
> The final version of the patch.
> 
> (r=paul, as per the previous comment.)
> 
> For reference, here's the final version of the patch, with the passing test.
> :)
> 
> I'll commit it once I figure out the landing procedure for m-c.  ;)

We will take care of that. And I want like to get a review from Dao as well (want to make sure there is no better option for this.
(Assignee)

Comment 14

6 years ago
Comment on attachment 588496 [details] [diff] [review]
The final version of the patch.

(Changing it to r?Paul, so that he can set the flag, and ask someone else for additional review.)
Attachment #588496 - Flags: review+ → review?(paul)
(Reporter)

Comment 15

6 years ago
Comment on attachment 588496 [details] [diff] [review]
The final version of the patch.

Dao, can you take a look this? I am wondering if we can have a better solution.
Attachment #588496 - Flags: review?(paul)
Attachment #588496 - Flags: review?(dao)
Attachment #588496 - Flags: review+
Comment on attachment 588496 [details] [diff] [review]
The final version of the patch.

>+#highlighter-nodeinfobar > span {
>+  display: inline;
>+  -moz-user-select: text;
>+  cursor: text;
> }

spans are inline by default.

>-    let tagNameLabel = this.chromeDoc.createElement("label");
>+    let tagNameLabel = this.chromeDoc.createElement("span");
>     tagNameLabel.id = "highlighter-nodeinfobar-tagname";
>     tagNameLabel.className = "plain";
> 
>-    let idLabel = this.chromeDoc.createElement("label");
>+    let idLabel = this.chromeDoc.createElement("span");
>     idLabel.id = "highlighter-nodeinfobar-id";
>     idLabel.className = "plain";
> 
>-    let classesBox = this.chromeDoc.createElement("hbox");
>+    let classesBox = this.chromeDoc.createElement("span");
>     classesBox.id = "highlighter-nodeinfobar-classes";
>+    classesBox.className = "plain";
>+    // Add some content to force a better boundingClientRect down below.
>+    classesBox.textContent = "&nbsp;";

The "plain" class isn't useful here anymore.

>--- a/browser/themes/winstripe/browser.css
>+++ b/browser/themes/winstripe/browser.css
>@@ -2710,17 +2710,18 @@ panel[dimmed="true"] {
> #highlighter-nodeinfobar-tagname {
>   color: white;
> }
> 
> #highlighter-nodeinfobar-id {
>   color: hsl(90, 79%, 52%);
> }
> 
>-.highlighter-nodeinfobar-class {
>+#highlighter-nodeinfobar,
>+#highlighter-nodeinfobar-classes {
>   color: hsl(200, 100%, 65%);
> }

Why both #highlighter-nodeinfobar and #highlighter-nodeinfobar-classes?
(In reply to Dão Gottwald [:dao] from comment #16)
> Comment on attachment 588496 [details] [diff] [review]
> The final version of the patch.
> 
> >+#highlighter-nodeinfobar > span {
> >+  display: inline;
> >+  -moz-user-select: text;
> >+  cursor: text;
> > }
> 
> spans are inline by default.
> 
> >-    let tagNameLabel = this.chromeDoc.createElement("label");
> >+    let tagNameLabel = this.chromeDoc.createElement("span");

Err, it looks like you're creating the spans in the XUL namespace...
Comment on attachment 588496 [details] [diff] [review]
The final version of the patch.

You have two options here: Create proper HTML nodes or just use XUL boxes (what you're effectively doing already).
Attachment #588496 - Flags: review?(dao) → review-
(Reporter)

Updated

6 years ago
Blocks: 717919, 717916
(Reporter)

Comment 19

6 years ago
This discussion can help:
http://groups.google.com/group/mozilla.dev.platform/browse_thread/thread/e315f48f7bffea3a#
(Assignee)

Comment 20

6 years ago
Created attachment 590020 [details] [diff] [review]
The, uh, one after the final version of the patch.  :)

Okay, I found that using XUL nodes didn't give me the results I wanted (the entire label would disappear and be replaced with "…", instead of only the end content being replaced).  And switching everything to spans also didn't do what I wanted (although I admit that I forget how now.  I suspect it was because I couldn't get the CSS to apply to the xhtml elements).  But, leaving the last element as a span seemed to make everything work out reasonably.

I removed the "plain" class on the last element.

I'm using both #highlighter-nodeinfobar and #highlighter-nodeinfobar-classes because the "text-overflow: ellipsis" is on the highlighter-nodeinfobar

Thanks,
Blake.
Attachment #588496 - Attachment is obsolete: true
Attachment #590020 - Flags: review?(dao)
(In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #20)
> And switching everything to spans also didn't
> do what I wanted (although I admit that I forget how now.  I suspect it was
> because I couldn't get the CSS to apply to the xhtml elements).

The default namespace in those CSS files is XUL. You can target HTML nodes with html|, e.g. html|*.foo.
Comment on attachment 590020 [details] [diff] [review]
The, uh, one after the final version of the patch.  :)

>+#highlighter-nodeinfobar > .plain {
>+  -moz-user-select: text;
>+  cursor: text;
> }

>-#highlighter-nodeinfobar-id,
>-.highlighter-nodeinfobar-class,
>-#highlighter-nodeinfobar-tagname {
>-  -moz-user-select: text;
>-  cursor: text;
>-}

Please undo this change. Use html|*#highlighter-nodeinfobar-classes ...

>+    classesBox.style.MozUserSelect = "text";

... and remove this line.

>+    classes.textContent = "";
> 
>     if (this.node.className) {
>-      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);
>-      }
>-      classes.appendChild(fragment);
>+      let fragment = [];
>+      for (let i = 0; i < this.node.classList.length; i++)
>+        fragment.push(".", this.node.classList[i]);
>+      classes.textContent = fragment.join("");
>     }

if (this.node.className) {...} else { classes.textContent = ""; }
Attachment #590020 - Flags: review?(dao) → review-
(Assignee)

Comment 23

6 years ago
Created attachment 592554 [details] [diff] [review]
The next version of the patch.

Okay, I've fixed the things you asked for, and the tests still pass, so here's what I've got so far.

Thanks,
Blake.
Attachment #590020 - Attachment is obsolete: true
Attachment #592554 - Flags: review?(dao)
Comment on attachment 592554 [details] [diff] [review]
The next version of the patch.

>     if (this.node.className) {
>-      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);
>-      }
>-      classes.appendChild(fragment);
>+      let fragment = [];
>+      for (let i = 0; i < this.node.classList.length; i++)
>+        fragment.push(".", this.node.classList[i]);
>+      classes.textContent = fragment.join("");
>+    } else {
>+      classes.textContent = "";
>     }

Can this be simplified to:

classes.textContent = this.node.classList.length ?
                        "." + Array.join(this.node.classList, ".") : "";

?
(Assignee)

Comment 25

6 years ago
Created attachment 592759 [details] [diff] [review]
A newer version of the patch.

(In reply to Dão Gottwald [:dao] from comment #24)
> Can this be simplified to:
> classes.textContent = this.node.classList.length ?
>                         "." + Array.join(this.node.classList, ".") : "";
> 
> ?

Yep, done.

I've also switched the id and tagname nodes to xhtml:spans, for the following two reasons:
1) The id could also be really long, and I believe we want it to crop similarly to the classes, as opposed to just vanishing.
2) If you triple-click the id or classes to select the whole line, and the tagname is a label, it won't automatically select the tagname, whereas if they are all spans, it will select the whole line.

Thanks,
Blake.
Attachment #592554 - Attachment is obsolete: true
Attachment #592554 - Flags: review?(dao)
Attachment #592759 - Flags: review?(dao)
Comment on attachment 592759 [details] [diff] [review]
A newer version of the patch.

>-#highlighter-nodeinfobar-id:empty {
>+html|*#highlighter-nodeinfobar-id:empty {
>   display: none;
> }

not sure what this is actually doing...

>-    let tagNameLabel = this.chromeDoc.createElement("label");
>+    let tagNameLabel = this.chromeDoc.createElementNS("http://www.w3.org/1999/xhtml", "span");
>     tagNameLabel.id = "highlighter-nodeinfobar-tagname";
>     tagNameLabel.className = "plain";
> 
>-    let idLabel = this.chromeDoc.createElement("label");
>+    let idLabel = this.chromeDoc.createElementNS("http://www.w3.org/1999/xhtml", "span");
>     idLabel.id = "highlighter-nodeinfobar-id";
>     idLabel.className = "plain";

remove the "plain" classes

>-.highlighter-nodeinfobar-class {
>-  color: hsl(200, 100%, 65%);
>-}
>-

> #highlighter-nodeinfobar {
>+  color: hsl(200, 100%, 65%);

Why this change?
Attachment #592759 - Flags: review?(dao) → review+
(Assignee)

Comment 27

6 years ago
(In reply to Dão Gottwald [:dao] from comment #26)
> >-#highlighter-nodeinfobar-id:empty {
> >+html|*#highlighter-nodeinfobar-id:empty {
> >   display: none;
> > }
> not sure what this is actually doing...

Me either, but it was there, so I figured I'ld leave it in.  (I'll check whether it's still needed, and if not, I'll remove it.)

> >     tagNameLabel.className = "plain";
> >     idLabel.className = "plain";
> remove the "plain" classes

Okay.

> >-.highlighter-nodeinfobar-class {
> >-  color: hsl(200, 100%, 65%);
> >-}
> >-
> 
> > #highlighter-nodeinfobar {
> >+  color: hsl(200, 100%, 65%);
> Why this change?

Because the "text-overflow: ellipsis" is on the highlighter-nodeinfobar, it gets its colour from that element, so to make it look more like the highlighter-nodeinfobar-class is the thing being truncated, I've set the highlighter-nodeinfobar's colour to the colour that used to be on the highlighter-nodeinfobar-class.  (This was the change I came up with in response to your previous question "Why both #highlighter-nodeinfobar and #highlighter-nodeinfobar-classes?", if that helps put it in context.)

Thanks,
Blake.
(Reporter)

Comment 28

6 years ago
Thank you Blake for this patch!

(In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #27)
> (In reply to Dão Gottwald [:dao] from comment #26)
> > >-#highlighter-nodeinfobar-id:empty {
> > >+html|*#highlighter-nodeinfobar-id:empty {
> > >   display: none;
> > > }
> > not sure what this is actually doing...
> 
> Me either, but it was there, so I figured I'ld leave it in.  (I'll check
> whether it's still needed, and if not, I'll remove it.)

This was to hide the '#' added with a pseudo element.
(Assignee)

Comment 29

6 years ago
Created attachment 592796 [details] [diff] [review]
The possibly actually final version of the patch.

(In reply to Paul Rouget [:paul] from comment #28)
> Thank you Blake for this patch!

My pleasure!  :)

> (In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #27)
> > > >-#highlighter-nodeinfobar-id:empty {
> > > >+html|*#highlighter-nodeinfobar-id:empty {
> > > >   display: none;
> > > > }
> > > not sure what this is actually doing...
> > Me either, but it was there, so I figured I'ld leave it in.  (I'll check
> > whether it's still needed, and if not, I'll remove it.)
> This was to hide the '#' added with a pseudo element.

Removed!  ;)

(We don't add the '#' with a pseudo element anymore, so this isn't needed anymore.)

Later,
Blake.
Attachment #592759 - Attachment is obsolete: true
(Reporter)

Updated

6 years ago
Whiteboard: [good first bug][mentor=paul][lang=css] → [good first bug][mentor=paul][lang=css][land-in-fx-team]
(Reporter)

Updated

6 years ago
Blocks: 724507
(Reporter)

Comment 30

6 years ago
https://hg.mozilla.org/integration/fx-team/rev/a650e8302840
Whiteboard: [good first bug][mentor=paul][lang=css][land-in-fx-team] → [good first bug][mentor=paul][lang=css][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/a650e8302840
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [good first bug][mentor=paul][lang=css][fixed-in-fx-team] → [good first bug][mentor=paul][lang=css]
Target Milestone: --- → Firefox 13
You need to log in before you can comment on or make changes to this bug.