Closed Bug 694958 Opened 13 years ago Closed 12 years ago

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

Categories

(DevTools :: General, defect, P3)

Product:
DevTools
Component:
General
Platform:
x86
macOS
Type:
defect

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Firefox 13

People

(Reporter: paul, Assigned: bwinton)

References

(
URL
)

Details

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

Attachments

(1 file, 8 obsolete files)

The possibly actually final version of the patch.
11.03 KB, patch
Details | Diff | Splinter Review 
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.
We're doing developer tool prioritization, filter on 'brontozaur'
to ignore the spam.
Priority: -- → P3
Whiteboard: [good first bug][mentor=paul][lang=css]
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.
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).
Attached patch The first cut at the patch. (obsolete) — — Splinter Review 
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)
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)?
(Added a better test url for an upcoming patch which gets us closer, I think.)
URL: <body><h1>Testing</h1><p id="testid" ... → <body><p id="shortid" class="short te...
Attached patch The second cut at the patch. (obsolete) — — Splinter Review 
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)
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>.
(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)
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)
Blocks: 708257
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)
Attached patch The final version of the patch. (obsolete) — — Splinter Review 
(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+
(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.
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)
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-
Blocks: 717919, 717916
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-
Attached patch The next version of the patch. (obsolete) — — Splinter Review 
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, ".") : "";

?
Attached patch A newer version of the patch. (obsolete) — — Splinter Review 
(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+
(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.
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.
(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
Whiteboard: [good first bug][mentor=paul][lang=css] → [good first bug][mentor=paul][lang=css][land-in-fx-team]
Blocks: 724507
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
Closed: 12 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
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: