The default bug view has changed. See this FAQ.

Uppercased text in element highlighter tooltip

RESOLVED FIXED in Firefox 50

Status

()

Firefox
Developer Tools: Inspector
P2
minor
RESOLVED FIXED
a year ago
8 months ago

People

(Reporter: Grisha Pushkov, Assigned: seban, Mentored)

Tracking

46 Branch
Firefox 50
Points:
---

Firefox Tracking Flags

(firefox50 fixed)

Details

(Whiteboard: [good first bug][lang=js][btpp-fix-later])

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

a year ago
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.
(Reporter)

Updated

a year ago
Component: Untriaged → Developer Tools: Inspector
(Reporter)

Comment 1

a year ago
Forgot screenshot (http://imgur.com/LwNRrKZ).

Updated

a year ago
Severity: normal → minor
Status: UNCONFIRMED → NEW
Ever confirmed: true
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
Mentor: pbrosset@mozilla.com
Priority: -- → P2
Whiteboard: [good first bug][lang=js][btpp-fix-later]

Comment 3

11 months ago
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.
Attachment #8747457 - Flags: review?(pbrosset)

Comment 4

11 months ago
Probably related to bug 1086532 (Make <style scoped> work in native anonymous nodes)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e5869bf96aa3

Comment 6

11 months ago
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.
Attachment #8747457 - Flags: review?(pbrosset)

Comment 7

10 months ago
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!
Flags: needinfo?(ale.comp_06)
(Assignee)

Comment 8

9 months ago
Hi Patrick,

As a.l.e is away can you assign this bug to me.
Flags: needinfo?(pbrosset)

Updated

9 months ago
Assignee: nobody → sebastinssanty
Flags: needinfo?(pbrosset)
Flags: needinfo?(ale.comp_06)
(Assignee)

Comment 9

9 months ago
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.
Flags: needinfo?(pbrosset)
Attachment #8767087 - Flags: review?(pbrosset)

Updated

9 months ago
Attachment #8747457 - Attachment is obsolete: true
Flags: needinfo?(pbrosset)
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.
Attachment #8767087 - Flags: review?(pbrosset)
(Assignee)

Comment 11

9 months ago
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.
Attachment #8767087 - Attachment is obsolete: true
Attachment #8767386 - Flags: review?(pbrosset)
(Assignee)

Updated

9 months ago
Status: NEW → ASSIGNED

Comment 12

9 months ago
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 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.
Attachment #8767386 - Flags: review?(pbrosset) → review+
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.
(Assignee)

Comment 15

9 months ago
Created attachment 8767892 [details] [diff] [review]
bug1235062_uppercasedtooltip.diff

Made the necessary changes.
Attachment #8767386 - Attachment is obsolete: true
Attachment #8767892 - Flags: review+
(Assignee)

Updated

9 months ago
Keywords: checkin-needed

Comment 16

9 months ago
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
Keywords: checkin-needed

Comment 17

9 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f4df6e5d0612
Status: ASSIGNED → RESOLVED
Last Resolved: 9 months ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50

Updated

8 months ago
Depends on: 1291306
You need to log in before you can comment on or make changes to this bug.