Last Comment Bug 1235062 - Uppercased text in element highlighter tooltip
: Uppercased text in element highlighter tooltip
Status: RESOLVED FIXED
[good first bug][lang=js][btpp-fix-la...
:
Product: Firefox
Classification: Client Software
Component: Developer Tools: Inspector (show other bugs)
: 46 Branch
: Unspecified Unspecified
P2 minor (vote)
: Firefox 50
Assigned To: Sebastin Santy [:seban]
:
: Gabriel Luong [:gl][1 biz day review guarantee] (ΦωΦ)
Mentors: Patrick Brosset <:pbro>
Depends on: 1291306
Blocks:
  Show dependency treegraph
 
Reported: 2015-12-24 07:01 PST by Grisha Pushkov
Modified: 2016-08-02 08:13 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed


Attachments
0001-Bug-1235062-Uppercased-text-in-element-highlighter-t.patch (732 bytes, patch)
2016-04-30 09:01 PDT, a.l.e
no flags Details | Diff | Splinter Review
bug1235062_uppercasedtooltip.diff (482 bytes, patch)
2016-07-01 01:28 PDT, Sebastin Santy [:seban]
no flags Details | Diff | Splinter Review
bug1235062_uppercasedtooltip.diff (721 bytes, patch)
2016-07-02 00:57 PDT, Sebastin Santy [:seban]
pbrosset: review+
Details | Diff | Splinter Review
bug1235062_uppercasedtooltip.diff (806 bytes, patch)
2016-07-05 02:16 PDT, Sebastin Santy [:seban]
sebastinssanty: review+
Details | Diff | Splinter Review

Description User image Grisha Pushkov 2015-12-24 07:01:29 PST
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.11; rv:45.0) Gecko/20100101 Firefox/45.0
Build ID: 20151222004010

Steps to reproduce:

Add `text-transform: uppercase` to html element. After that when selecting element in inspector, text in tooltip (id or class) is uppercased.
Comment 1 User image Grisha Pushkov 2015-12-24 07:26:08 PST
Forgot screenshot (http://imgur.com/LwNRrKZ).
Comment 2 User image Patrick Brosset <:pbro> 2016-04-08 13:12:05 PDT
Good catch! Should be a really easy fix if someone want to try this as their first bug. I can mentor them.
Make sure you first go through our contribution doc: https://developer.mozilla.org/en-US/docs/Tools/Contributing
Comment 3 User image a.l.e 2016-04-30 09:01:59 PDT
Created attachment 8747457 [details] [diff] [review]
0001-Bug-1235062-Uppercased-text-in-element-highlighter-t.patch

a nice patch from the mozilla hackathon in zurich.
Comment 4 User image a.l.e 2016-04-30 09:04:09 PDT
Probably related to bug 1086532 (Make <style scoped> work in native anonymous nodes)
Comment 5 User image Michael Kohler [:mkohler] 2016-04-30 15:06:20 PDT
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e5869bf96aa3
Comment 6 User image Patrick Brosset <:pbro> 2016-05-02 01:03:11 PDT
Comment on attachment 8747457 [details] [diff] [review]
0001-Bug-1235062-Uppercased-text-in-element-highlighter-t.patch

Review of attachment 8747457 [details] [diff] [review]:
-----------------------------------------------------------------

This is a good start, and I like the comments before and after the reset rule, so it stands out from the rest, and it's clear what it's for. However, I've seen a couple of mistakes. See below.

::: devtools/server/actors/highlighters.css
@@ +13,5 @@
>    element.
>  */
>  
> +/*
> +  Until 1086532 is fixed, reset the formatting that have been defined for

Hmm, I don't think this bug will help in fact.
<style scoped> will make sure that the highlighter styles don't leak into the content page, but won't help at all prevent content style affecting the highlighter.
We still need to make sure the highlighter's styles don't get affected by styles set on html.

@@ +15,5 @@
>  
> +/*
> +  Until 1086532 is fixed, reset the formatting that have been defined for
> +  <html> and are not overwritten in this file.
> +  This fixes 1235062.

I have a feeling we'll end up with quite a list of properties being reset here, so no need to list the bug number this fixes here, cause there will be more in the future, and we can always use mercurial logs to find out bug numbers.

@@ +19,5 @@
> +  This fixes 1235062.
> +*/
> +
> +:-moz-native-anonymous {
> +  text-transform: unset;

unset doesn't work for me, initial does though.
Comment 7 User image Jan Keromnes [:janx] 2016-06-08 08:49:36 PDT
Hi a.l.e, thanks again for your patch! I just wanted to let you know that Patrick reviewed it (see comment 6), in case you're interested in following up on it. But if you don't have time, that's ok too, your initial patch already helped a lot!
Comment 8 User image Sebastin Santy [:seban] 2016-07-01 00:50:04 PDT
Hi Patrick,

As a.l.e is away can you assign this bug to me.
Comment 9 User image Sebastin Santy [:seban] 2016-07-01 01:28:34 PDT
Created attachment 8767087 [details] [diff] [review]
bug1235062_uppercasedtooltip.diff

Hi Patrick,

Thanks for assigning this bug to me. I have noticed that this error is only noticed when changing rules of <html>. I have made the changes after going through the comments over here, and it is working perfectly. I want to know whether any further additions should be made.
Comment 10 User image Patrick Brosset <:pbro> 2016-07-01 07:37:36 PDT
Comment on attachment 8767087 [details] [diff] [review]
bug1235062_uppercasedtooltip.diff

Review of attachment 8767087 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, but while we're at it, I found 3 other properties that impacted the highlighter:
open this page in firefox and highlight any node in the page:

<!DOCTYPE html>
<style>
html {
  text-transform: uppercase;
  text-indent: 100px;
  letter-spacing: 9px;
  word-spacing: 17px;
}
</style>

You'll see that the text-transform property is indeed re-initialized thanks to your patch, but text-indent, letter-spacing and word-spacing all impact the text in the highlighter.
Could you also take care of these ones please?

::: devtools/server/actors/highlighters.css
@@ +13,4 @@
>    element.
>  */
>  
> +:-moz-native-anonymous {

Please add a comment before this rule that explains that this rule is here to avoid content style from impacting highlighters.
Comment 11 User image Sebastin Santy [:seban] 2016-07-02 00:57:01 PDT
Created attachment 8767386 [details] [diff] [review]
bug1235062_uppercasedtooltip.diff

Update to the previous patch: Properties mentioned by you have been set to initial. The comment is also added.
Comment 12 User image a.l.e 2016-07-04 01:20:19 PDT
hi
sorry for the late reply.
sadly, my skills concerning the firefox code are not enough for more than an easy fix...
anyway, nice to see that somebody could improve the patch!
Comment 13 User image Patrick Brosset <:pbro> 2016-07-05 01:10:55 PDT
Comment on attachment 8767386 [details] [diff] [review]
bug1235062_uppercasedtooltip.diff

Review of attachment 8767386 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, this looks good.
I've made a couple of minor comments below. Please address them and re-upload a patch here. You don't need to ask for a review again, you can simply set R+ yourself on the new patch when it's ready. No need for me to re-review these final changes since they are minor.

Also, one final thing: please format the patch correctly with a commit message and an author name.
The commit message should be along these lines:
Bug <number> - <short description of the change>; r=<reviewer>

So in your case:
Bug 1235062 - Set some highlighter CSS properties to initial values to avoid content CSS leaking; r=pbro

See more info about this and how to format the patch here: https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

::: devtools/server/actors/highlighters.css
@@ +14,5 @@
>  */
>  
> +:-moz-native-anonymous {
> +  /*
> +  The formatting of html element (for content styling) is impacting the highlighters. 

There is a trailing whitespace at the end of this line, please remove it.
Also, I'm not sure this comment is really correct. I'd maybe rephrase it to:

Content CSS applying to the html element impact the highlighters.
To avoid that, possible cases have been set to initial.

@@ +15,5 @@
>  
> +:-moz-native-anonymous {
> +  /*
> +  The formatting of html element (for content styling) is impacting the highlighters. 
> +  To avoid that, possible cases have been set to initial. 

There is a trailing whitespace at the end of this line, please remove it.
Comment 14 User image Patrick Brosset <:pbro> 2016-07-05 01:17:21 PDT
Also, I forgot to mention, I pushed your patch to the TRY server:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=28abab366ee6
When all the build and test jobs finish and are green, that means this patch didn't cause any regressions in the current tests and we're safe to land it.
Comment 15 User image Sebastin Santy [:seban] 2016-07-05 02:16:13 PDT
Created attachment 8767892 [details] [diff] [review]
bug1235062_uppercasedtooltip.diff

Made the necessary changes.
Comment 16 User image Pulsebot 2016-07-05 20:56:08 PDT
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/f4df6e5d0612
Set some highlighter CSS properties to initial values to avoid content CSS leaking; r=pbro
Comment 17 User image Carsten Book [:Tomcat] 2016-07-07 02:45:16 PDT
https://hg.mozilla.org/mozilla-central/rev/f4df6e5d0612

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