Last Comment Bug 704223 - styles added in data urls are displayed badly in the rule view
: styles added in data urls are displayed badly in the rule view
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Developer Tools: Inspector (show other bugs)
: unspecified
: x86 Mac OS X
: P2 normal (vote)
: Firefox 13
Assigned To: Blake Winton (:bwinton) (:☕️)
:
: Patrick Brosset <:pbro>
Mentors:
http://dl.dropbox.com/u/2301433/Scree...
Depends on:
Blocks: 708257
  Show dependency treegraph
 
Reported: 2011-11-21 11:13 PST by Byron Jones ‹:glob› [PTO until 2017-01-09]
Modified: 2012-03-12 12:59 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
screenshot (157.75 KB, image/png)
2011-11-21 11:13 PST, Byron Jones ‹:glob› [PTO until 2017-01-09]
no flags Details
A first cut at the patch. (791 bytes, patch)
2011-12-23 11:31 PST, Blake Winton (:bwinton) (:☕️)
no flags Details | Diff | Splinter Review
The second cut at the patch. (6.65 KB, patch)
2012-01-03 18:48 PST, Blake Winton (:bwinton) (:☕️)
no flags Details | Diff | Splinter Review
The cut-down version of the patch. (6.53 KB, patch)
2012-01-13 14:41 PST, Blake Winton (:bwinton) (:☕️)
no flags Details | Diff | Splinter Review
The patch, with only the used bits. (5.12 KB, patch)
2012-02-05 11:32 PST, Blake Winton (:bwinton) (:☕️)
no flags Details | Diff | Splinter Review
A newer version of the patch. (4.15 KB, patch)
2012-03-06 08:19 PST, Blake Winton (:bwinton) (:☕️)
no flags Details | Diff | Splinter Review
The (hopefully) last version of the patch. (791 bytes, patch)
2012-03-08 17:34 PST, Blake Winton (:bwinton) (:☕️)
paul: review+
Details | Diff | Splinter Review

Description Byron Jones ‹:glob› [PTO until 2017-01-09] 2011-11-21 11:13:25 PST
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 Dave Camp (:dcamp) 2011-11-21 11:16:58 PST
Pretty nasty.  This will affect rule and style views.
Comment 2 Blake Winton (:bwinton) (:☕️) 2011-12-23 11:31:14 PST
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.
Comment 3 Paul Rouget [:paul] 2012-01-03 03:48:37 PST
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.
Comment 4 Blake Winton (:bwinton) (:☕️) 2012-01-03 18:48:55 PST
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.
Comment 5 Blake Winton (:bwinton) (:☕️) 2012-01-03 18:49:38 PST
Oh, and I'm using the file in the url for a test.
Comment 6 Michael Ratcliffe [:miker] [:mratcliffe] 2012-01-11 04:13:02 PST
Bug triage, filter on PEGASUS.
Comment 7 Paul Rouget [:paul] 2012-01-13 11:42:50 PST
(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!
Comment 8 Blake Winton (:bwinton) (:☕️) 2012-01-13 14:41:40 PST
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.
Comment 9 Paul Rouget [:paul] 2012-01-17 06:12:36 PST
The function source() is never used. Please update the patch to keep only the code that is actually needed.
Comment 10 Blake Winton (:bwinton) (:☕️) 2012-02-05 11:32:57 PST
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.
Comment 11 Paul Rouget [:paul] 2012-03-05 20:40:50 PST
Why do we need to use the `reallyShort` argument? Can't we assume that we always want to shrink the dataURLs?
Comment 12 Blake Winton (:bwinton) (:☕️) 2012-03-06 08:19:25 PST
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.
Comment 13 Paul Rouget [:paul] 2012-03-07 11:16:37 PST
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.
Comment 14 Blake Winton (:bwinton) (:☕️) 2012-03-08 17:34:00 PST
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.
Comment 15 Paul Rouget [:paul] 2012-03-09 01:49:31 PST
Thanks!
Comment 16 Panos Astithas [:past] 2012-03-12 04:15:58 PDT
https://hg.mozilla.org/integration/fx-team/rev/1d8fdbc69641
Comment 17 Tim Taubert [:ttaubert] 2012-03-12 12:59:46 PDT
https://hg.mozilla.org/mozilla-central/rev/1d8fdbc69641

Note You need to log in before you can comment on or make changes to this bug.