Closed Bug 1189464 Opened 10 years ago Closed 9 years ago

[markup-view] Closing tag is always black after "Edit As HTML"

Categories

(DevTools :: Inspector, defect)

Product:
DevTools
Component:
Inspector
Type:
defect
Priority:
Not set
Severity:
minor

Tracking

(firefox42 affected, firefox44 fixed)

Status:
VERIFIED FIXED
Milestone:
Firefox 44
Tracking Flags:
Tracking Status
firefox42 --- affected
firefox44 --- fixed

People

(Reporter: arni2033, Assigned: akrawchyk, Mentored)

References

(
URL
)

Details

(Whiteboard: [good first bug][lang=js][polish-backlog])

Attachments

(1 file, 1 obsolete file)

Removes the fg theme contrast from closing tags after expanding
890 bytes, patch
bgrins
: review+
Details | Diff | Splinter Review
STR (tested on Win7, Nightly 42.0a1 (2015-07-29), fresh profile): 1. Set dark devtools theme 2. Open page data:text/html,<body> 3. Inspect <body> 4. Right-click <body> and select "Edit As HTML" 5. Type string "<body><div></div></body>" instead of "<body></body>" 6. Press Ctrl+Enter to save changes RESULT: Text "body" in closing tag is black EXPECTATIONS: It should be visible. NOTE: After editing as HTML the text in closing tag gains className "theme-fg-contrast" and therefore changes color. I think, black closing tag is OK for light devtools theme, and in this bug we should only change CSS of dark theme: .theme-fg-contrast{color: black;} to {color:white;} See chrome://browser/skin/devtools/dark-theme.css , line 211 (for Nightly 42.0a1 (2015-07-29))
Component: Theme → Developer Tools: Inspector
I believe the 'theme-fg-contrast' class should not be added at all in this case (also not in the light theme), because it makes the start and end tag look different. Sebastian
Severity: normal → minor
OS: Unspecified → All
Hardware: Unspecified → All
Summary: (Inspector) Closing tag is always black after "Edit As HTML" → [markup-view] Closing tag is always black after "Edit As HTML"
Whiteboard: [good first bug][lang=js]
I would like to work on this as my first bug :) I updated https://dxr.mozilla.org/mozilla-central/source/devtools/client/themes/dark-theme.css#213 to `color: white;`, however I agree with Sebastian that this does not fix the issue, it changes the end tag from black to white, see example http://i.imgur.com/aTbY5pa.gif. Should I explore changing the logic in https://dxr.mozilla.org/mozilla-central/source/devtools/client/markupview/markup-view.js#1989 to include the end tag?
Flags: needinfo?(pbrosset)
(In reply to Sebastian Zartner [:sebo] from comment #1) > 'theme-fg-contrast' should not be added at all, because it makes the start and end tag look different What does mockup say? Maybe it was designer's choice to make closing tag look different to indicate that innerHTML was edited? Maybe asking the person who implemented this - is the better way? (In reply to akrawchyk from comment #2) > this does not fix the issue I'd say, the only issue here is closing tag being invisible, and setting color:white fixes the issue. It's always hard to tell what is right in any case w/o mockups.
Attached patch Bug1189464.patch (obsolete) — — Splinter Review
Attachment #8671437 - Flags: review?(pbrosset)
Flags: needinfo?(pbrosset)
Comment on attachment 8671437 [details] [diff] [review] Bug1189464.patch Review of attachment 8671437 [details] [diff] [review]: ----------------------------------------------------------------- Forwarding this Brian who's a better reviewer for this.
Attachment #8671437 - Flags: review?(pbrosset) → review?(bgrinstead)
(In reply to Sebastian Zartner [:sebo] from comment #1) > I believe the 'theme-fg-contrast' class should not be added at all in this > case (also not in the light theme), because it makes the start and end tag > look different. I agree. It's a bug that the closing tag ends up with that class applied, not part of the design. Looks like an issue where a node is being cloned whie it has the highlight class applied to it. See: https://dxr.mozilla.org/mozilla-central/source/devtools/client/markupview/markup-view.js#1829. You should be able to call `flashElementOff(line);` before assigning it to this.closeTagLine
Comment on attachment 8671437 [details] [diff] [review] Bug1189464.patch Review of attachment 8671437 [details] [diff] [review]: ----------------------------------------------------------------- We should make sure that .theme-fg-contrast is removed from the closing tag instead of changing the style. See comment 6
Attachment #8671437 - Flags: review?(bgrinstead)
Thanks for working on this, by the way. Let me know if you have questions
Assignee: nobody → akrawchyk
Status: NEW → ASSIGNED
Whiteboard: [good first bug][lang=js] → [good first bug][lang=js][polish-backlog]
Attachment #8671437 - Attachment is obsolete: true
Attachment #8672303 - Flags: review?(bgrinstead)
Comment on attachment 8672303 [details] [diff] [review] Removes the fg theme contrast from closing tags after expanding Review of attachment 8672303 [details] [diff] [review]: ----------------------------------------------------------------- This looks good, I've added some test coverage for this in another patch and pushed to the test server: https://treeherder.mozilla.org/#/jobs?repo=try&revision=93d739e3c4da
Attachment #8672303 - Flags: review?(bgrinstead) → review+
https://hg.mozilla.org/mozilla-central/rev/e1273c74b483 https://hg.mozilla.org/mozilla-central/rev/f9ab40dfbe99
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox44: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 44
Thanks akrawchyk! This is now fixed in Nightly
(In reply to Brian Grinstead [:bgrins] from comment #14) > Thanks akrawchyk! This is now fixed in Nightly You betcha! I appreciate the help :)
I can confirm that it's now working as expected. Thanks! Having said this, I may point to the regression related to the HTML editor in bug 1215418. Sebastian
Status: RESOLVED → VERIFIED
(In reply to Sebastian Zartner [:sebo] from comment #16) > I can confirm that it's now working as expected. Thanks! > > Having said this, I may point to the regression related to the HTML editor > in bug 1215418. > > Sebastian Thanks for the report. I've confirmed locally that backing out this commit does *not* fix the problem, so it must be something else
Has STR: --- → yes
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: