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

Status: VERIFIED FIXED

(Core :: DOM: UI Events & Focus Handling, defect)

Description

[Dão Gottwald [:dao]]

Attachment: patch

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.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

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 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 3

[Dão Gottwald [:dao]]

Attachment: patch with helper function

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 5

[Dão Gottwald [:dao]]

Attachment: patch

as suggested

Comment 6

[Mike Connor [:mconnor]]

Comment on attachment 286387 [details] [diff] [review]
patch

r=me on the browser bits.

a=endgame drivers for M9 checkin

Comment 7

[Reed Loden [:reed]]

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

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Dao: Thanks for doing this!

Comment 9

[pal-moz]

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

[Boris Zbarsky [:bzbarsky]]

pal-moz: in what app?

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

Comment 11

[pal-moz]

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

[Boris Zbarsky [:bzbarsky]]

> (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

[pal-moz]

(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 14

[Reed Loden [:reed]]

Try with 5 (with this patch, of course).

Comment 15

[Boris Zbarsky [:bzbarsky]]

Ah, yeah.  That's what you want.

Comment 16

[pal-moz]

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

[Boris Zbarsky [:bzbarsky]]

Right.  The point is that Firefox no longer supports text-only zoom, as of the checkin for bug 389628.

Comment 18

[pal-moz]

(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

[Boris Zbarsky [:bzbarsky]]

> 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

[pal-moz]

ok.

BTW if text-only zoom is no longer supported, bug 401322 should be marked as "INVALID or WONTFIX" ?

Comment 21

[Martijn Wargers (dead)]

No, that one can be fixed, if someone throws in patch.

Comment 22

[pal-moz]

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

[Boris Zbarsky [:bzbarsky]]

> Boris says "Firefox no longer supports text-only zoom" in comment#17.

That's not an irrevocable decision.

Comment 24

[Martijn Wargers (dead)]

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.