Skip to content
This repository was 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 all commits
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
42 changes: 21 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 ratio of line-height to font-size when they use the same units
* @type {float}
*/
var LINE_HEIGHT = 1.25;

/**
* @private
* @type {PreferenceStorage}
Expand Down Expand Up @@ -147,28 +155,20 @@ 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. The minimum font size is 1px or 0.1em
// and the maximum font size is 72px or 7.2em depending on the unit used
if (fsNew < MIN_FONT_SIZE * delta || fsNew > MAX_FONT_SIZE * delta) {
Copy link
Contributor

@redmunds redmunds May 7, 2013

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

@TomMalbran TomMalbran May 7, 2013

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

@redmunds redmunds May 7, 2013

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