Dão Gottwald [:dao] Assignee
	Description • 17 years ago

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.

	Olli Pettay [:smaug][bugs@pettay.fi]
	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.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 2 • 17 years ago

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.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 4 • 17 years ago

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.

	Dão Gottwald [:dao] Assignee
	Comment 5 • 17 years ago

as suggested

	Mike Connor [: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

	Reed Loden [:reed]
	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

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 8 • 17 years ago

Dao: Thanks for doing this!

	pal-moz
	Comment 9 • 17 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 ?

	Boris Zbarsky [:bzbarsky]
	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?

	pal-moz
	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).

	Boris Zbarsky [:bzbarsky]
	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).

	pal-moz
	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.

	Reed Loden [:reed]
	Comment 14 • 17 years ago

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

	Boris Zbarsky [:bzbarsky]
	Comment 15 • 17 years ago

Ah, yeah.  That's what you want.

	pal-moz
	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.

	Boris Zbarsky [:bzbarsky]
	Comment 17 • 17 years ago

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

	pal-moz
	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 ?

	Boris Zbarsky [:bzbarsky]
	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.

	pal-moz
	Comment 20 • 17 years ago

ok.

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

	Martijn Wargers (dead)
	Comment 21 • 17 years ago

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

	pal-moz
	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" ?

	Boris Zbarsky [:bzbarsky]
	Comment 23 • 17 years ago

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

That's not an irrevocable decision.

	Martijn Wargers (dead)
	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.