If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Text zoom with scroll mouse doesn't obey minimum/maximum zoom properly (momentary exceed allowed)

RESOLVED FIXED

Status

()

Core
Event Handling
--
trivial
RESOLVED FIXED
10 years ago
9 years ago

People

(Reporter: David Cattran, Assigned: Siraj Razick)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

10 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b5pre) Gecko/2008032304 Minefield/3.0b5pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b5pre) Gecko/2008032304 Minefield/3.0b5pre

When using text zoom (option checked in view menu), and using the mouse scroll buttons to zoom, the zoom will be allowed both higher and lower than the maximum and minimum zoom.  Then it seems to realize that you've exceeded the limit, and redraws at the min/max, or sometimes a step above or below the min or max.

If you zoom out quite quickly, for example, the page can flip down several zoom levels, and appear very small, then flip back to the maximum zoom level.

Contrast that to the full zoom, which will not zoom outside the limits at all.  It's rock solid.  

Reproducible: Always

Steps to Reproduce:
1. Set text zoom View->Zoom->Text Zoom Only
2. Navigate to a text-heavy page
3. Use Ctrl-Mouse Scroll to zoom to minimum and maximum
4. Attempt to zoom beyond minimum and maximum
Actual Results:  
Observe flash to incorrect zoom level, then back to correct min or max

Expected Results:  
Should not display at an out of range zoom level at all, it should check the zoom level before refreshing the first time.  Try this in full zoom mode, and the correct behaviour is observed.
(Reporter)

Updated

10 years ago
Version: unspecified → Trunk

Comment 1

9 years ago
Confirming, I see this as well.

Looks like nsEventStateManager::ChangeTextSize() should take zoom.minPercent and zoom.maxPercent into account, like ChangeFullZoom() does:

http://mxr.mozilla.org/mozilla-central/source/content/events/src/nsEventStateManager.cpp#2062
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows XP → All
Hardware: PC → All
(Assignee)

Comment 2

9 years ago
Created attachment 344196 [details] [diff] [review]
Inital patch based on Elmar's comment 

With this patch nsEventStateManager::ChangeTextSize() takes zoom.minPercent
and zoom.maxPercent into account, like ChangeFullZoom() does, like Elmar suggested. I'm not sure who should review this, but marking r?.
Attachment #344196 - Flags: superreview?
Attachment #344196 - Flags: review?

Comment 3

9 years ago
Patch looks fine to me, but I'm not a reviewer.

I would probably name the two new variables "zoomMin" and "zoomMax" (like in ChangeFullZoom), since they apply to both zoom types. You also need to fix the indentation of:

  mv->SetTextZoom(textzoom);

About finding appropriate reviewers, see these pages:

http://www.mozilla.org/owners.html#document-object-model
http://www.mozilla.org/hacking/reviewers.html

I'd try peterv or jst for r? and bz for sr?
Component: General → Event Handling
Product: Firefox → Core
QA Contact: general → events
(Assignee)

Comment 4

9 years ago
Comment on attachment 344196 [details] [diff] [review]
Inital patch based on Elmar's comment 

Marking the patch obsolete till I fix the indentation and the variable names suggested by  Elmar. I'll attach a new one right now.
Attachment #344196 - Attachment is obsolete: true
Attachment #344196 - Flags: superreview?
Attachment #344196 - Flags: review?
(Assignee)

Comment 5

9 years ago
Created attachment 344259 [details] [diff] [review]
Make nsEventStateManager::ChangeTextSize() takes zoom.minPercent and zoom.maxPercent into account

sEventStateManager::ChangeTextSize() takes zoom.minPercent
and zoom.maxPercent into account. and this version Adds the changes suggested by Elmar for the first patch a.) Fix indentation b.) Change variable names .
Attachment #344259 - Flags: superreview?(peterv)
Attachment #344259 - Flags: review?(bzbarsky)
Comment on attachment 344259 [details] [diff] [review]
Make nsEventStateManager::ChangeTextSize() takes zoom.minPercent and zoom.maxPercent into account

Looks good.
Attachment #344259 - Flags: superreview?(peterv)
Attachment #344259 - Flags: superreview+
Attachment #344259 - Flags: review?(bzbarsky)
Attachment #344259 - Flags: review+
Pushed as changeset ed515d20c255.  Siraj Razick, thank you for the patch!
Assignee: nobody → siraj.razick
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
(Assignee)

Comment 8

9 years ago
(In reply to comment #7)
> Pushed as changeset ed515d20c255.  Siraj Razick, thank you for the patch!

Thank you for taking the time for review it :)

Comment 9

9 years ago
Nit, I'd prefer C++ casting.
You need to log in before you can comment on or make changes to this bug.