Closed
Bug 401214
Opened 17 years ago
Closed 17 years ago
bring back support for MOUSE_SCROLL_TEXTSIZE (mousewheel.withcontrolkey.action = 3)
Categories
(Core :: DOM: UI Events & Focus Handling, defect)
Core
DOM: UI Events & Focus Handling
Tracking
()
VERIFIED
FIXED
mozilla1.9beta1
People
(Reporter: dao, Assigned: dao)
References
Details
Attachments
(1 file, 2 obsolete files)
(deleted),
patch
|
mconnor
:
review+
mconnor
:
approvalM9+
|
Details | Diff | Splinter Review |
bug 389628 comment #129: > As of the checkin of this patch it's impossible to do textZoom in response to a > mousewheel event, unless I'm seriously misreading the patch. bug 389628 comment #130: > As a simple example of the sort of thing this patch breaks, the relevant > mousewheel option in Seamonkey preferences is labeled "Make the text larger or > smaller". Which is no longer what the behavior will be, with this patch. Now > maybe the Seamonkey developers want the full zoom behavior (and will just > change the wording). Or maybe they want text zoom. They just no longer get a > choice in what they get, with this patch. And given the lack of public > discussion, they probably also don't know that a core API that used to do one > thing now does something totally different and that they need to worry about > either changing their UI or something. Note that in the patch ChangeTextSize and DoScrollTextsize are identical to how these functions looked like before bug 389628 landed.
Flags: blocking1.9?
Attachment #286270 -
Flags: superreview?(jonas)
Attachment #286270 -
Flags: review?(jonas)
Assignee | ||
Updated•17 years ago
|
Attachment #286270 -
Flags: review?(mconnor)
Comment 1•17 years ago
|
||
Comment on attachment 286270 [details] [diff] [review] patch Um, you're duplicating quite a bit code in ESM. Maybe you could somehow merge DoScrollTextsize and DoScrollFullZoom, and ChangeTextSize and ChangeFullZoom, or at least you could make a helper function to get nsIMarkupDocumentViewer.
Comment on attachment 286270 [details] [diff] [review] patch Yeah, please reduce the code duplication in ChangeTextSize/ChangeFullZoom DoScrollTextsize is fine though IMHO, not much to win there.
Attachment #286270 -
Flags: review?(jonas) → review-
Assignee | ||
Comment 3•17 years ago
|
||
Attachment #286270 -
Attachment is obsolete: true
Attachment #286348 -
Flags: superreview?(jonas)
Attachment #286348 -
Flags: review?(jonas)
Attachment #286270 -
Flags: superreview?(jonas)
Attachment #286270 -
Flags: review?(mconnor)
Assignee | ||
Updated•17 years ago
|
Attachment #286348 -
Flags: review?(mconnor)
Comment on attachment 286348 [details] [diff] [review] patch with helper function >+nsEventStateManager::ChangeTextSize(PRInt32 change) >+{ >+ nsCOMPtr<nsIMarkupDocumentViewer> mv; >+ if (NS_SUCCEEDED(GetMarkupDocumentViewer(getter_AddRefs(mv)))) { >+ float textzoom; >+ mv->GetTextZoom(&textzoom); >+ textzoom += ((float)change) / 10; >+ if (textzoom > 0 && textzoom <= 20) >+ mv->SetTextZoom(textzoom); >+ return NS_OK; >+ } >+ return NS_ERROR_FAILURE; >+} Use the following pattern instead: { nsCOMPtr<nsIMarkupDocumentViewer> mv; nsresult rv = GetMarkupDocumentViewer(getter_AddRefs(mv)); NS_ENSURE_SUCCESS(rv, rv); ... return NS_OK } That makes for much more readable code in the long run. r/sr=me with that fixed at both callsites.
Attachment #286348 -
Flags: superreview?(jonas)
Attachment #286348 -
Flags: superreview+
Attachment #286348 -
Flags: review?(jonas)
Attachment #286348 -
Flags: review+
Assignee | ||
Comment 5•17 years ago
|
||
as suggested
Attachment #286348 -
Attachment is obsolete: true
Attachment #286387 -
Flags: review?(mconnor)
Attachment #286387 -
Flags: approvalM9?
Attachment #286348 -
Flags: review?(mconnor)
Comment 6•17 years ago
|
||
Comment on attachment 286387 [details] [diff] [review] patch r=me on the browser bits. a=endgame drivers for M9 checkin
Attachment #286387 -
Flags: review?(mconnor)
Attachment #286387 -
Flags: review+
Attachment #286387 -
Flags: approvalM9?
Attachment #286387 -
Flags: approvalM9+
Assignee | ||
Updated•17 years ago
|
Keywords: checkin-needed
Updated•17 years ago
|
Flags: blocking1.9? → blocking1.9+
Comment 7•17 years ago
|
||
Checking in browser/app/profile/firefox.js; /cvsroot/mozilla/browser/app/profile/firefox.js,v <-- firefox.js new revision: 1.216; previous revision: 1.215 done Checking in browser/base/content/browser-textZoom.js; /cvsroot/mozilla/browser/base/content/browser-textZoom.js,v <-- browser-textZoom.js new revision: 1.5; previous revision: 1.4 done Checking in content/events/src/nsEventStateManager.cpp; /cvsroot/mozilla/content/events/src/nsEventStateManager.cpp,v <-- nsEventStateManager.cpp new revision: 1.716; previous revision: 1.715 done Checking in content/events/src/nsEventStateManager.h; /cvsroot/mozilla/content/events/src/nsEventStateManager.h,v <-- nsEventStateManager.h new revision: 1.160; previous revision: 1.159 done
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Updated•17 years ago
|
Keywords: checkin-needed
Dao: Thanks for doing this!
after ctrl+wheel, cannot reset with ctrl+0. (ctrl+0 is used for full page zoom.) ctrl+0 should be used for both (full page zoom and text zoom) ? if cannot use it for both, new way to reset should be created ?
![]() |
||
Comment 10•17 years ago
|
||
pal-moz: in what app? If Firefox, does the pref value in Firefox need to be changed to start doing full zoom?
Comment 11•17 years ago
|
||
Firefox. (mousewheel.withcontrolkey.action set/change to 3) after full page zoom (View>Zoom or ctrl++/ctrl+-), you can reset with ctrl+0. but after text-zoom (ctrl+mouse wheel), you cannot reset text size with ctrl+0. now no way to reset text size after text-zoom (ctrl+mousewheel).
![]() |
||
Comment 12•17 years ago
|
||
> (mousewheel.withcontrolkey.action set/change to 3)
"Don't do that". You want 4. "3" is basically unsupported by the Firefox UI (as you discovered).
Comment 13•17 years ago
|
||
(In reply to comment #12) > > (mousewheel.withcontrolkey.action set/change to 3) > > "Don't do that". You want 4. "3" is basically unsupported by the Firefox UI > (as you discovered). > if set to "4", cannot use ctrl+mousewheel.
Comment 16•17 years ago
|
||
if 5, ctrl+mousewheel is full page zoom, cannot zoom only text. if 3, ctrl++/- or View>Zoom is full page zoom, and ctrl+mousewheel is for text zoom. full page zoom can be reset by ctrl+0, but text zoom cannot be reset by ctrl+0.
![]() |
||
Comment 17•17 years ago
|
||
Right. The point is that Firefox no longer supports text-only zoom, as of the checkin for bug 389628.
Comment 18•17 years ago
|
||
(In reply to comment #17) > Right. The point is that Firefox no longer supports text-only zoom, as of the > checkin for bug 389628. > I know. BTW, if text-only zoom is not supported, what is this bug for ? for re-enable text-only zoom ? "MOUSE_SCROLL_TEXTSIZE" means you can text-only zoom by ctrl+mousewheel ?
![]() |
||
Comment 19•17 years ago
|
||
> BTW, if text-only zoom is not supported, what is this bug for ?
It's not supported in Firefox. This bug is about Gecko.
Comment 20•17 years ago
|
||
ok. BTW if text-only zoom is no longer supported, bug 401322 should be marked as "INVALID or WONTFIX" ?
Comment 22•17 years ago
|
||
why? Boris says "Firefox no longer supports text-only zoom" in comment#17. if bug 401322 will be fixed, it means "Firefox supports text-only zoom" ?
![]() |
||
Comment 23•17 years ago
|
||
> Boris says "Firefox no longer supports text-only zoom" in comment#17.
That's not an irrevocable decision.
Comment 24•17 years ago
|
||
Verified fixed, using: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a9pre) Gecko/2007110405 Minefield/3.0a9pre mousewheel.withcontrolkey.action;3 sets ctrl-mousewheel action back to the textZoom option.
Status: RESOLVED → VERIFIED
Updated•6 years ago
|
Component: Event Handling → User events and focus handling
You need to log in
before you can comment on or make changes to this bug.
Description
•