Uppercased text in element highlighter tooltip

RESOLVED FIXED in Firefox 50

Status

defect
P2
minor
RESOLVED FIXED
4 years ago
Last year

People

(Reporter: grisha, Assigned: seban, Mentored)

Tracking

46 Branch
Firefox 50
Dependency tree / graph

Firefox Tracking Flags

(firefox50 fixed)

Details

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

Attachments

(1 attachment, 3 obsolete attachments)

Reporter

Description

4 years 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

4 years ago
Component: Untriaged → Developer Tools: Inspector
Reporter

Comment 1

4 years ago
Forgot screenshot (http://imgur.com/LwNRrKZ).

Updated

4 years 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
Priority: -- → P2
Whiteboard: [good first bug][lang=js][btpp-fix-later]

Comment 3

3 years ago
a nice patch from the mozilla hackathon in zurich.
Attachment #8747457 - Flags: review?(pbrosset)

Comment 4

3 years ago
Probably related to bug 1086532 (Make <style scoped> work in native anonymous nodes)
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)
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

3 years ago
Hi Patrick,

As a.l.e is away can you assign this bug to me.
Flags: needinfo?(pbrosset)
Assignee: nobody → sebastinssanty
Flags: needinfo?(pbrosset)
Flags: needinfo?(ale.comp_06)
Assignee

Comment 9

3 years ago
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)
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

3 years ago
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

3 years ago
Status: NEW → ASSIGNED

Comment 12

3 years 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

3 years ago
Made the necessary changes.
Attachment #8767386 - Attachment is obsolete: true
Attachment #8767892 - Flags: review+
Assignee

Updated

3 years ago
Keywords: checkin-needed

Comment 16

3 years 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

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f4df6e5d0612
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50

Updated

Last year
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.