Closed
Bug 694958
Opened 13 years ago
Closed 13 years ago
[highlighter] [infobar] limit the size of the infobar
Categories
(DevTools :: General, defect, P3)
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 13
People
(Reporter: paul, Assigned: bwinton)
References
()
Details
(Whiteboard: [good first bug][mentor=paul][lang=css])
Attachments
(1 file, 8 obsolete files)
|
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.
|
||
Comment 1•13 years ago
|
||
We're doing developer tool prioritization, filter on 'brontozaur'
to ignore the spam.
Priority: -- → P3
|
Reporter | |
Updated•13 years ago
|
Whiteboard: [good first bug][mentor=paul][lang=css]
|
Assignee | |
Comment 2•13 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•13 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•13 years ago
|
|
|
Assignee | |
Comment 4•13 years ago
|
||
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.
|
|
Reporter | |
Comment 5•13 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•13 years ago
|
||
(Added a better test url for an upcoming patch which gets us closer, I think.)
|
|
Assignee | |
Comment 7•13 years ago
|
||
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•13 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•13 years ago
|
||
(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•13 years ago
|
||
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 | |
Comment 11•13 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•13 years ago
|
||
(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•13 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•13 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•13 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 16•13 years ago
|
||
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 = " ";
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?
|
|
||
Comment 17•13 years ago
|
||
(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 18•13 years ago
|
||
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•13 years ago
|
|
|
Reporter | |
Comment 19•13 years ago
|
||
This discussion can help:
http://groups.google.com/group/mozilla.dev.platform/browse_thread/thread/e315f48f7bffea3a#
|
|
Assignee | |
Comment 20•13 years ago
|
||
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)
|
|
||
Comment 21•13 years ago
|
||
(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 22•13 years ago
|
||
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•13 years ago
|
||
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 24•13 years ago
|
||
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•13 years ago
|
||
(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 26•13 years ago
|
||
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•13 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•13 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•13 years ago
|
||
(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•13 years ago
|
Whiteboard: [good first bug][mentor=paul][lang=css] → [good first bug][mentor=paul][lang=css][land-in-fx-team]
|
|
Reporter | |
Comment 30•13 years ago
|
||
Whiteboard: [good first bug][mentor=paul][lang=css][land-in-fx-team] → [good first bug][mentor=paul][lang=css][fixed-in-fx-team]
|
||
Comment 31•13 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 13 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
|
||
Updated•7 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•