Closed Bug 865448 Opened 7 years ago Closed 6 years ago

Breakpoint icon not vertically centered on line

Categories

(DevTools :: Debugger, defect, P3, trivial)

x86
macOS
defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 29

People

(Reporter: basta, Assigned: lie.r.min.g)

Details

(Whiteboard: [sourceeditor])

Attachments

(2 files, 3 obsolete files)

http://cl.ly/image/3h0x210v0P0M

You can see that the blue breakpoint icon is not centered on the line. Instead, the top of the icon is aligned with the top of the line. Since the icon is taller than one line, it bleeds a little bit down to the next line.
Ideally we'd want to fix bug 762979, but improving the current icon would be OK, too.
Priority: -- → P3
Whiteboard: [sourceeditor]
Attached patch 865448.patch (obsolete) — Splinter Review
Attachment #822712 - Flags: review?(mihai.sucan)
Comment on attachment 822712 [details] [diff] [review]
865448.patch

Thank you for the patch!

For me the breakpoint icon is correctly aligned without this patch. When I apply this patch the icon becomes misaligned. See:

http://i.imgur.com/J5zIIQL.png

Maybe you need to set the line height equal to the height of the line numbers gutter, then vertically align the icon.
Attachment #822712 - Flags: review?(mihai.sucan)
Attached patch 865448.patch (obsolete) — Splinter Review
Trying different approach here as both the line number and debug icon container are individually positioned as absolute.
Attachment #833303 - Flags: review?(mihai.sucan)
Comment on attachment 833303 [details] [diff] [review]
865448.patch

This patch changes codemirror.js and I am not the right person to review such changes. Plus, I cannot reproduce the problem this bug report mentions.

Anton, can you please check this? Do we fork codemirror?

Thanks!
Attachment #833303 - Flags: review?(mihai.sucan) → review?(anton)
Comment on attachment 833303 [details] [diff] [review]
865448.patch

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

Why do we need to modify both CSS and codemirror.js files? We should avoid modifying codemirror.js since it makes upgrades more difficult. And in this case it seems like a CSS change (maybe with !important?) will do the trick.
The reason I changed the js is because of the dom element.

div.CodeMirror-Code
--div {position:relative} // The line of code container
---- div {position: absolute; left:0px; } ^a
------ div.CodeMirror-gutter-elt.CodeMirror-linenumber {position: absolute; left:16px; } ^b
------ div.CodeMirror-gutter-elt {position: absolute; left:0px; } ^c
---- pre // Actual line content ^d

I need to target ^a but there is no class name for it. I can target it using [.CodeMirror-code > div > div] but i do not know whether that one will be preferable than putting the style directly from the js file.

We are trying to vertically align ^b, ^c and ^d. I don't think we can use vertical-align as both ^b and ^c are positioned as absolute. I cannot align ^b and ^c using top/translate without first vertically centering ^a. When I used top:50% from ^b and ^c (.CodeMirror-gutter-elt), it will calculate top as top:0px (height of ^a is 0px as both its children is an absolute positioned element).
Yes, targeting .CodeMirror-code > div > div (or even :first?) is preferable than editing the CodeMirror's source file.
Attachment #833303 - Flags: review?(anton)
Attached patch 865448.patch (obsolete) — Splinter Review
Attachment #822712 - Attachment is obsolete: true
Attachment #833303 - Attachment is obsolete: true
Attachment #8335466 - Flags: review?(anton)
Attached patch 865448.patchSplinter Review
Sorry, forgot to do a hq qref. Here is the one with the correct selector
Attachment #8335466 - Attachment is obsolete: true
Attachment #8335466 - Flags: review?(anton)
Attachment #8335468 - Flags: review?(anton)
Comment on attachment 8335468 [details] [diff] [review]
865448.patch

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

You'll need to change r=msucan to r=anton so that people could blame the correct person if something goes wrong. :)
Attachment #8335468 - Flags: review?(anton) → review+
Attached patch 865448.patchSplinter Review
Attachment #8335555 - Flags: review+
we have a patch and a review. Can we land these? Thanks!
Status: NEW → ASSIGNED
https://hg.mozilla.org/integration/fx-team/rev/7cd75f021d3a
Whiteboard: [sourceeditor] → [fixed-in-fx-team] [sourceeditor]
https://hg.mozilla.org/mozilla-central/rev/7cd75f021d3a
Assignee: nobody → lie.r.min.g
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team] [sourceeditor] → [sourceeditor]
Target Milestone: --- → Firefox 29
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.