styles added in data urls are displayed badly in the rule view

RESOLVED FIXED in Firefox 13

Status

()

Firefox
Developer Tools: Inspector
P2
normal
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: glob, Assigned: bwinton)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 13
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments, 5 obsolete attachments)

(Reporter)

Description

6 years ago
Created attachment 575916 [details]
screenshot

the stylish addon (https://addons.mozilla.org/en-US/firefox/addon/stylish/) adds styles via a data: url.

styles which have been added via this addon are displayed very poorly in the style editor -- see the attached screenshot.

Comment 1

6 years ago
Pretty nasty.  This will affect rule and style views.
Priority: -- → P2

Updated

6 years ago
Summary: styles added by stylish are displayed badly in the style editor → styles added in data urls are displayed badly in the style editor

Updated

6 years ago
Whiteboard: [good first bug][mentor=dcamp]

Updated

6 years ago
Component: Developer Tools: Style Editor → Developer Tools: Inspector
QA Contact: developer.tools.style.editor → developer.tools.inspector
Summary: styles added in data urls are displayed badly in the style editor → styles added in data urls are displayed badly in the style inspector
(Assignee)

Comment 2

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

So, this seemed to be the simplest thing that would work.  I can see a lot of potential room for improvement (i.e. restricting the data url to a single line, but still showing the full text as a tooltop), but I didn't want to go way overboard until I heard some of your thoughts.  (And perhaps this patch is enough of an improvement to check in…)

Thanks,
Blake.
Assignee: nobody → bwinton
Status: NEW → ASSIGNED
Attachment #584093 - Flags: feedback?(dcamp)

Comment 3

6 years ago
Looks good. I think this is enough.

One single line is going to be tricky, because we don't want the other href to be cropped.
Adding the complete data url as a tooltip would be nice though.
(Assignee)

Comment 4

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

(In reply to Paul Rouget [:paul] from comment #3)
> Looks good. I think this is enough.

Cool.  :)

> One single line is going to be tricky, because we don't want the other href
> to be cropped.
> Adding the complete data url as a tooltip would be nice though.

So, I tried using a tooltiptext attribute (with and without namespaces), and a title attribute (again, with and without namespaces), and neither of them got a tooltip to show up, so I've fallen back on the classic hidden-absolute-span-that-gets-shown-on-hover trick.

If anyone (*cough* Paul *cough* ;) feels like playing around to see if they can get the simpler title/tooltiptext stuff working, I'ld love to hear what I did wrong.  If not, this at least gets us closer.

Thanks,
Blake.
Attachment #584093 - Attachment is obsolete: true
Attachment #584093 - Flags: feedback?(dcamp)
Attachment #585628 - Flags: feedback?(dcamp)
(Assignee)

Comment 5

6 years ago
Oh, and I'm using the file in the url for a test.

Updated

6 years ago
Blocks: 708257
Whiteboard: [good first bug][mentor=dcamp] → [styleinspector][good first bug][mentor=dcamp]
Bug triage, filter on PEGASUS.
Whiteboard: [styleinspector][good first bug][mentor=dcamp] → [computedview][ruleview][good first bug][mentor=dcamp]

Updated

6 years ago
Summary: styles added in data urls are displayed badly in the style inspector → styles added in data urls are displayed badly in the rule view

Comment 7

6 years ago
(In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #4)
> > One single line is going to be tricky, because we don't want the other href
> > to be cropped.
> > Adding the complete data url as a tooltip would be nice though.
> 
> So, I tried using a tooltiptext attribute (with and without namespaces), and
> a title attribute (again, with and without namespaces), and neither of them
> got a tooltip to show up, so I've fallen back on the classic
> hidden-absolute-span-that-gets-shown-on-hover trick.
> 
> If anyone (*cough* Paul *cough* ;) feels like playing around to see if they
> can get the simpler title/tooltiptext stuff working, I'ld love to hear what
> I did wrong.  If not, this at least gets us closer.

Let's get the tooltip in a followup bug. Can you just cut the dataurls? I want to get this asap. Thx!
(Assignee)

Comment 8

6 years ago
Created attachment 588531 [details] [diff] [review]
The cut-down version of the patch.

I noticed that I didn't handle the HtmlTree version of this, so here's the patch, with no tooltips, which also handles that.

Later,
Blake.
Attachment #585628 - Attachment is obsolete: true
Attachment #585628 - Flags: feedback?(dcamp)
Attachment #588531 - Flags: review?(paul)

Comment 9

6 years ago
The function source() is never used. Please update the patch to keep only the code that is actually needed.
(Assignee)

Comment 10

6 years ago
Created attachment 594571 [details] [diff] [review]
The patch, with only the used bits.

Okay, here's the patch without the source function.  Sorry it took so long to get this posted.

Thanks,
Blake.
Attachment #588531 - Attachment is obsolete: true
Attachment #588531 - Flags: review?(paul)
Attachment #594571 - Flags: review?(paul)

Comment 11

6 years ago
Why do we need to use the `reallyShort` argument? Can't we assume that we always want to shrink the dataURLs?
(Assignee)

Comment 12

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

> Why do we need to use the `reallyShort` argument? Can't we assume that we
> always want to shrink the dataURLs?

Well, I didn't want to make that assumption, because I'm not totally familiar with all the interactions in the code, but if you feel it's okay, I'm happy to do it.  And here's the patch that implements that.  :)

As a side note, I think this might make it hard to figure out which data url is being referenced, if there's more than one, since they're all reduced to "data:text/css" now.  I couldn't see a way of getting the line number of the link, but if we could, perhaps we want to change them to "data:text/css at line 43", or something…  But that seems like we might want to handle it in a followup bug.

Thanks,
Blake.
Attachment #594571 - Attachment is obsolete: true
Attachment #594571 - Flags: review?(paul)
Attachment #603284 - Flags: review?(paul)

Comment 13

6 years ago
I am not sure to understand why you need to introduce shortSource.

I think only this part of the patch is actually useful:

diff --git a/browser/devtools/styleinspector/CssLogic.jsm b/browser/devtools/styleinspector/CssLogic.jsm
--- a/browser/devtools/styleinspector/CssLogic.jsm
+++ b/browser/devtools/styleinspector/CssLogic.jsm
@@ -922,17 +922,18 @@ CssLogic.shortSource = function CssLogic
   if (url.filePath) {
     return url.filePath;
   }
 
   if (url.query) {
     return url.query;
   }
 
-  return aSheet.href;
+  let dataUrl = aSheet.href.match(/^(data:[^,]*),/);
+  return dataUrl ? dataUrl[1] : aSheet.href;
 }
 
 /**
  * A safe way to access cached bits of information about a stylesheet.
  *
  * @constructor
  * @param {CssLogic} aCssLogic pointer to the CssLogic instance working with
  * this CssSheet object.
(Assignee)

Comment 14

6 years ago
Created attachment 604269 [details] [diff] [review]
The (hopefully) last version of the patch.

You know, I could have sworn that I needed it for something, but compiling without it, I can't find any differences, so here's the simplest patch.

(I also ran the tests, and they all seemed to pass.  Let me know if you want me to write a new test for this fix.)

Thanks,
Blake.
Attachment #603284 - Attachment is obsolete: true
Attachment #603284 - Flags: review?(paul)
Attachment #604269 - Flags: review?(paul)

Updated

6 years ago
Attachment #604269 - Flags: review?(paul) → review+

Comment 15

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