Scratchpad Line numbers don't line up

RESOLVED FIXED in Firefox 17

Status

()

Firefox
Developer Tools: Scratchpad
P2
normal
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: rc, Assigned: capella)

Tracking

({regression})

unspecified
Firefox 18
regression
Points:
---

Firefox Tracking Flags

(firefox17 verified)

Details

Attachments

(4 attachments)

(Reporter)

Description

6 years ago
Regression introduced with the landing of bug 756423. On Mac, the line numbers no longer line up due to the additional height of the command key on Macs.
(Reporter)

Comment 1

6 years ago
Created attachment 648083 [details]
scratchpad screenshot
(Reporter)

Comment 2

6 years ago
One possible solution for this would be to change the string returned for the Command Key to "Command" or "Cmd".
Ouch. This happens because Orion, at initialization, checks the line height and it assumes all chars have the same height. Unfortunately the Command Key character is of a different height, breaking a lot of things in Orion (not just line numbers alignment).

Good suggestion - we should just say "Command" or "Cmd".
So if the user types or pastes this (or a similar) character, Orion will lose it again? Would using the height of this character (or a similar "big" one) for initialization work?
I noticed some fonts have different "big" characters. we can't know which is the biggest char in each font. The actual solution is to fix how Orion works internally. Afaik, they will allow variable line heights in the future.

Additionally, if you don't use any of the big chars in your lines but you determine the line height using a big char during init.... you still have the same problem.
(Reporter)

Comment 6

6 years ago
looks like mihai is going to file upstream. We should do the keystring fix mentioned above in the meantime.

Filter on BLACKEAGLE?!
Priority: -- → P3
(Reporter)

Updated

6 years ago
Priority: P3 → P2
Changing "⌘" to "Cmd" wouldn't solve the problem, since the user can type this character.
On the other hand, changing the font we use to Menlo, which is the monospaced font that
Apple wants everybody to have as a default anyway, would fix it for good.
Besides, it is a great font.
Created attachment 649048 [details] [diff] [review]
Using the Menlo font fixes the issue.

For some reason, using this fix makes my font look big.
Does anyone have the same issue?
Blocks: 762164
(Reporter)

Comment 10

6 years ago
Comment on attachment 649048 [details] [diff] [review]
Using the Menlo font fixes the issue.

unfortunately, we can't do that. We should respect the system's font-settings. I would personally love to use Menlo, but if a user picks a different font...
(Reporter)

Comment 11

6 years ago
and, fwiw, my own system has Menlo specified here.
No longer blocks: 762164
(Assignee)

Comment 12

6 years ago
I just noticed this.

Until orion ever opts to go for variable line height, I'd like to change this to use "Cmd" to at least fix the regression caused by my patch.

This still follows Mac convention, and if the user inserts a cloverleaf char into the text, the problem is of his own doing, and is an exposure that currently exists on the orion side / not of our doing.
(Reporter)

Comment 13

6 years ago
(In reply to Mark Capella [:capella] from comment #12)
> I just noticed this.
> 
> Until orion ever opts to go for variable line height, I'd like to change
> this to use "Cmd" to at least fix the regression caused by my patch.
> 
> This still follows Mac convention, and if the user inserts a cloverleaf char
> into the text, the problem is of his own doing, and is an exposure that
> currently exists on the orion side / not of our doing.

yeah, I'd like that until we get a proper fix for line-heights. I think that's the way to go. Feel like taking it?
(Assignee)

Updated

6 years ago
Assignee: nobody → markcapella
Status: NEW → ASSIGNED
(Assignee)

Comment 14

6 years ago
Created attachment 654863 [details] [diff] [review]
Patch (v1)

Temp Fix - Override Mac |cloverleaf| char with |Cmd-| string, until Orion fixes variable line height.
Attachment #654863 - Flags: review?(rcampbell)
(Reporter)

Comment 15

6 years ago
Comment on attachment 654863 [details] [diff] [review]
Patch (v1)

+      // Use "Cmd-" literal vs cloverleaf meta-key until
+      // Orion adds variable height lines
+      //elemString += keysbundle.GetStringFromName("VK_META_CMD") +
+      //              keysbundle.GetStringFromName("MODIFIER_SEPARATOR");

could you reference this bug in the comment? You can preface with a XXX bug 779642 on your first comment line. Also add a space between the comment // and elemString.

r+ with those nits fixed.
Attachment #654863 - Flags: review?(rcampbell) → review+
https://hg.mozilla.org/mozilla-central/rev/98a07ac71ddc
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 18
We need to fix this in Aurora as well, right?
status-firefox17: --- → affected
(Reporter)

Comment 20

6 years ago
Comment on attachment 655794 [details] [diff] [review]
Patch (v2)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 756423
User impact if declined: users's line numbers won't line up in the scratchpad.
Testing completed (on m-c, etc.): on m-c, try
Risk to taking this patch (and alternatives if risky): users won't like the scratchpad.
String or UUID changes made by this patch: none (internal, non l10n strings only)
Attachment #655794 - Flags: approval-mozilla-aurora?
Comment on attachment 655794 [details] [diff] [review]
Patch (v2)

[Triage Comment]
New feature, early in the cycle, low risk fix. Approved for Aurora 17.
Attachment #655794 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Keywords: verifyme
Mozilla/5.0 (X11; Linux i686; rv:17.0) Gecko/17.0 Firefox/17.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:17.0) Gecko/17.0 Firefox/17.0

Verified with Ubuntu, Mac OS 10.7 and Windows 7. No problems with rendering and lining up characters with line numbers if the apple key character isn't used.
status-firefox17: fixed → verified
mass remove verifyme requests greater than 4 months old
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.