Closed Bug 672146 Opened 8 years ago Closed 8 years ago

Ensure that valid arguments are passed to Zoom Manager when 100% is not within the allowed min-max range

Categories

(SeaMonkey :: UI Design, defect)

defect
Not set

Tracking

(seamonkey2.2 wontfix, seamonkey2.3 fixed, seamonkey2.4 fixed)

RESOLVED FIXED
seamonkey2.5
Tracking Status
seamonkey2.2 --- wontfix
seamonkey2.3 --- fixed
seamonkey2.4 --- fixed

People

(Reporter: rsx11m.pub, Assigned: rsx11m.pub)

Details

Attachments

(1 file, 2 obsolete files)

Per bug 621823 comment #30, toolkit's Zoom Manager throws an exception when trying to set a zoom level which is outside the range specified by the zoom.{min,max}Percent preferences. There are a couple of cases where the implementation assumes implicitly that 100% is always within that range, specifically for the default case when no global value from the content pref is given, and when the Ctrl+0 keyboard shortcut is used (toolkit bug 539417). While 100% will be excluded as menu item when it's outside the allowed range after bug 621823 now, the remaining cases yet have to be fixed. Setting zoom.minPercent > 100% has been an accessibility use case to define a default global zoom (equally affecting text and figures) as a replacement for larger font settings or DPI changes (which won't scale figures and some formats), but now does no longer work.
Attached patch Proposed fix (obsolete) — Splinter Review
This patch adds this._ensureValid(1) where previously 1 was simply passed as argument without any checks. The first hunk takes care of the Ctrl+0 case and solves bug 539417 for SeaMonkey; the second hunk covers the handling of default cases. In both instances, the actual value used is adjusted either to the lower or the higher end to stay within the min-max range, thus avoiding that setZoomForBrowser() throws an exception. Note that globalValue is checked against min/max already in its get() function.
Assignee: nobody → rsx11m.pub
Status: NEW → ASSIGNED
Attachment #546455 - Flags: review?(neil)
Is is me or is it possible for _ensureValid to return an invalid value?
I didn't plan to touch that function, but apparently it will always return /some/ integer (e.g., "1" if the input is NaN). The only conflict I can see is the case where zoom.minPercent > zoom.maxPercent, where it will return ZoomManager.MIN as it's the earlier test. Such a case is probably rather a configuration error, not really much can be done if the preferences are selected in a conflicting way.
(In reply to comment #3)
> I didn't plan to touch that function, but apparently it will always return
> /some/ integer (e.g., "1" if the input is NaN).
This bug is about using "1" without checking that it's valid. So I'm concerned that this is another case of that bug...
I can make it "undefined" instead for that case, which wouldn't be valid return value in a strict sense either. The other option may be to treat it as aValue = 1 instead and let it go through the min/max checks as if it was the input value.
Attached patch Proposed fix (v2) (obsolete) — Splinter Review
Same as attachment 546455 [details] [diff] [review] but also subjects a NaN value to the min/max test.
Attachment #546455 - Attachment is obsolete: true
Attachment #546526 - Flags: review?(neil)
Attachment #546455 - Flags: review?(neil)
Comment on attachment 546526 [details] [diff] [review]
Proposed fix (v2)

>+    var testValue = aValue;
> 
>+    if (isNaN(testValue))
>+      testValue = 1;
Makes sense, but no need to make a copy of aValue, just set aValue = 1;
Attachment #546526 - Flags: review?(neil) → review+
I've learned that messing with function arguments (even if passed by value) is a bad style, but as you wish. ;-) Thanks for the quick review!
Attachment #546526 - Attachment is obsolete: true
Attachment #546542 - Flags: review+
Keywords: checkin-needed
Whiteboard: [c-n: comm-central]
Could someone give the patch a friendly push on comm-central (along with any other patches in the checkin-needed queue), please?

I was planning to nominate this for comm-aurora/beta, assuming that fixing even obscured exceptions qualifies and that it's simple enough, but that requires it to land on trunk first...
Comment on attachment 546542 [details] [diff] [review]
Patch for checkin [Checked in: trunk Comment 10 & comm-aurora/beta Comment 11]

http://hg.mozilla.org/comm-central/rev/bfd229071b88
Attachment #546542 - Attachment description: Patch for checkin → Patch for checkin [Checked in: Comment 10]
Attachment #546542 - Flags: approval-comm-beta+
Attachment #546542 - Flags: approval-comm-aurora+
Comment on attachment 546542 [details] [diff] [review]
Patch for checkin [Checked in: trunk Comment 10 & comm-aurora/beta Comment 11]

http://hg.mozilla.org/releases/comm-aurora/rev/5129a00ec84c
http://hg.mozilla.org/releases/comm-beta/rev/4c97611e9940
Attachment #546542 - Attachment description: Patch for checkin [Checked in: Comment 10] → Patch for checkin [Checked in: trunk Comment 10 & comm-aurora/beta Comment 11]
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Flags: in-testsuite-
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [c-n: comm-central]
Target Milestone: --- → seamonkey2.3
Thanks Ian!
Target Milestone: seamonkey2.3 → seamonkey2.5
Callek, this landed on Beta in time for 2.3, so why did you change TM to 2.5? I find that confusing.
(In reply to comment #13)
> Callek, this landed on Beta in time for 2.3, so why did you change TM to
> 2.5? I find that confusing.

Target milestone reflects what trunk was when it landed "there", branch-approvals get status-X:fixed.
You need to log in before you can comment on or make changes to this bug.