bring back support for MOUSE_SCROLL_TEXTSIZE (mousewheel.withcontrolkey.action = 3)

VERIFIED FIXED in mozilla1.9beta1

Status

()

defect
--
major
VERIFIED FIXED
12 years ago
2 months ago

People

(Reporter: dao, Assigned: dao)

Tracking

Trunk
mozilla1.9beta1
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Assignee

Description

12 years ago
Posted patch patch (obsolete) — 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

12 years ago
Attachment #286270 - Flags: review?(mconnor)
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

12 years ago
Posted patch patch with helper function (obsolete) — Splinter Review
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

12 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

12 years ago
Posted patch patchSplinter Review
as suggested
Attachment #286348 - Attachment is obsolete: true
Attachment #286387 - Flags: review?(mconnor)
Attachment #286387 - Flags: approvalM9?
Attachment #286348 - Flags: review?(mconnor)

Updated

12 years ago
Blocks: 401322
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

12 years ago
Keywords: checkin-needed
Flags: blocking1.9? → blocking1.9+
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
Last Resolved: 12 years ago
Resolution: --- → FIXED

Comment 9

12 years ago
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 ?
pal-moz: in what app?

If Firefox, does the pref value in Firefox need to be changed to start doing full zoom?

Comment 11

12 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).
> (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

12 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.
Try with 5 (with this patch, of course).
Ah, yeah.  That's what you want.

Comment 16

12 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.
Right.  The point is that Firefox no longer supports text-only zoom, as of the checkin for bug 389628.

Comment 18

12 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 ?
> 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

12 years ago
ok.

BTW if text-only zoom is no longer supported, bug 401322 should be marked as "INVALID or WONTFIX" ?
No, that one can be fixed, if someone throws in patch.

Comment 22

12 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" ?
> Boris says "Firefox no longer supports text-only zoom" in comment#17.

That's not an irrevocable decision.
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

12 years ago
Depends on: 405374
Component: Event Handling → User events and focus handling
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.