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

Status: RESOLVED FIXED

(DevTools :: Inspector, defect, P2)

Description

[:glob ✱]

Attachment: 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)]

Pretty nasty.  This will affect rule and style views.

Comment 2

[Blake Winton (:bwinton) (:☕️)]

Attachment: 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]]

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) (:☕️)]

Attachment: 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) (:☕️)]

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

Comment 6

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Bug triage, filter on PEGASUS.

Comment 7

[Paul Rouget [:paul]]

(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) (:☕️)]

Attachment: 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]]

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

Comment 10

[Blake Winton (:bwinton) (:☕️)]

Attachment: 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]]

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) (:☕️)]

Attachment: 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]]

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) (:☕️)]

Attachment: 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]]

Thanks!

Comment 16

[Panos Astithas (he/him) [:past] (please ni?)]

https://hg.mozilla.org/integration/fx-team/rev/1d8fdbc69641

Comment 17

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/1d8fdbc69641