Skip to content
This repository has been archived by the owner on Sep 6, 2021. It is now read-only.

Fix #3478: Persistent highlight is taller than transient highlight #3568

Merged
merged 3 commits into from
May 8, 2013
Merged

Fix #3478: Persistent highlight is taller than transient highlight #3568

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
42fae4a
Make thw line height be 1.25 times taller than the font-size when req…
TomMalbran Apr 23, 2013
5ce2eb3
Min max check comment updated
TomMalbran May 7, 2013
9f9a752
Line height to font size ratio comment updated
TomMalbran May 8, 2013
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 20 additions & 21 deletions src/view/ViewCommandHandlers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,15 @@ define(function (require, exports, module) {
* @type {int}
*/
var MAX_FONT_SIZE = 72;


/**
* @const
* @private
* The proportion between the font-size and the line-height in pixels
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just noticed that his comment is not quite correct. The operands are reversed, and it's not in pixels (it's a ratio).

It should be "The ratio of line-height to font-size".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, I intended to use it just for px, but then I realized that it would be required for ems too, when both units are ems. I'll update this comment too.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even better: "The ratio of line-height to font-size when font-size is in pixels"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually it is used when both font-size and line-height are in px or ems, and is not when they are different. If the font-size is on px and the line-height in ems, the line height doesn't change. So the previous one is more accurate.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It still bugs me that the operands are reversed. How about: "The ratio of line-height to font-size when the use the same units"

* @type {float}
*/
var LINE_HEIGHT = 1.25;

/**
* @private
* @type {PreferenceStorage}
Expand Down Expand Up @@ -147,28 +155,19 @@ define(function (require, exports, module) {
// Guaranteed to work by the validation above.
var fsUnits = fsStyle.substring(fsStyle.length - 2, fsStyle.length);
var lhUnits = lhStyle.substring(lhStyle.length - 2, lhStyle.length);

var fsOld = parseFloat(fsStyle.substring(0, fsStyle.length - 2));
var lhOld = parseFloat(lhStyle.substring(0, lhStyle.length - 2));

var fsDelta = (fsUnits === "px") ? adjustment : (0.1 * adjustment);
var lhDelta = (lhUnits === "px") ? adjustment : (0.1 * adjustment);

var fsNew = fsOld + fsDelta;
var lhNew = lhOld + lhDelta;
var delta = (fsUnits === "px") ? 1 : 0.1;

var fsStr = fsNew + fsUnits;
var lhStr = lhNew + lhUnits;

// Don't let the font size get too small.
if ((fsUnits === "px" && fsNew < MIN_FONT_SIZE) ||
(fsUnits === "em" && fsNew < (MIN_FONT_SIZE * 0.1))) {
return false;
}
var fsOld = parseFloat(fsStyle.substring(0, fsStyle.length - 2));
var lhOld = parseFloat(lhStyle.substring(0, lhStyle.length - 2));

var fsNew = fsOld + (delta * adjustment);
var lhNew = (fsUnits === lhUnits) ? fsNew * LINE_HEIGHT : lhOld;

// Don't let the font size get too large.
if ((fsUnits === "px" && fsNew > MAX_FONT_SIZE) ||
(fsUnits === "em" && fsNew > (MAX_FONT_SIZE * 0.1))) {
var fsStr = fsNew + fsUnits;
var lhStr = lhNew + lhUnits;

// Don't let the font size get too small or too large.
if (fsNew < MIN_FONT_SIZE * delta || fsNew > MAX_FONT_SIZE * delta) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maximum px size is 72, otherwise 7.2. This works for px and em, but if anyone changes units to anything else, the results may not be desirable. Maybe this should check for units type (like it did before).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing would actually work for other units, and that is why the code is returned on line 151 when the units aren't px or em, so this check wasn't that necessary. If a new font size unit would be introduced the whole code would need to be updated to support it, so I don't think this is an issue.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, this is a bit cryptic, so please explain the 72 vs 7.2 in a comment.

return false;
}

Expand Down