Closed Bug 1235062 Opened 8 years ago Closed 8 years ago

Uppercased text in element highlighter tooltip

Categories

(DevTools :: Inspector, defect, P2)

Product:
DevTools
Component:
Inspector
Version:
46 Branch
Type:
defect

Tracking

(firefox50 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 50
Tracking Flags:
Tracking Status
firefox50 --- fixed

People

(Reporter: grisha, Assigned: seban, Mentored)

References

Details

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

Attachments

(1 file, 3 obsolete files)

bug1235062_uppercasedtooltip.diff
806 bytes, patch
seban
: review+
Details | Diff | Splinter Review 
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.
Component: Untriaged → Developer Tools: Inspector
Forgot screenshot (http://imgur.com/LwNRrKZ).
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]
a nice patch from the mozilla hackathon in zurich.
Attachment #8747457 - Flags: review?(pbrosset)
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)
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)
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)
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)
Status: NEW → ASSIGNED
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.
Made the necessary changes.
Attachment #8767386 - Attachment is obsolete: true
Attachment #8767892 - Flags: review+
Keywords: checkin-needed
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
https://hg.mozilla.org/mozilla-central/rev/f4df6e5d0612
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
Depends on: 1291306
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: