RickieES aka Ricardo Palomares Reporter
	Description • 13 years ago

SeaMonkey should also have buttons and icons for feed preview

	joshB
	Comment 2 • 6 years ago

Is this still an active bug? Could I work on it?

	Frank-Rainer Grahl (:frg)
	Comment 3 • 6 years ago

> Is this still an active bug?

Yes. Personally I would probably prefer a zoom slider in the status bar but that might not be trivial. 

> Could I work on it?

Sure just attach a patch. Be aware that the current comm-central trunk is currently mostly broken. I would do it on comm-esr52 and rebase the patch then. Doing it on comm-beta should work too and this will become the next release but it has some major problems too. They are tracked in bug 1433370.

	Felix Härnström Assignee
	Comment 4 • 6 years ago

I would be interested in working on this. I would just like som input on whether it should be implementad as separate zoom in/out buttons, or a single button with a dropdown. The dropdown reduces clutter of course, but is a bit counterintuitive if the whole purpose is easy access without having to go into menus.

Thoughts?

	Ian Neal
	Comment 5 • 6 years ago

Zoom slider as long as it is easy to set 100% is my preference but other options would be a combined dropdown/text box or +/- buttons. I think it depends what widgets are already available, I don't remember seeing one that can be used as a slider but I may be wrong.

	Felix Härnström Assignee
	Comment 6 • 6 years ago

(In reply to Ian Neal from comment #5)
> Zoom slider as long as it is easy to set 100% is my preference but other
> options would be a combined dropdown/text box or +/- buttons. I think it
> depends what widgets are already available, I don't remember seeing one that
> can be used as a slider but I may be wrong.

Yeah, the slider seems to be non-trivial compared to the simple button or menu options. Could be wrong though, will take a closer look when I start digging into it a bit more. Good idea on adding both a menu option, or separate +/- buttons. I might run with that and see what I come up with.

	Frank-Rainer Grahl (:frg)
	Comment 7 • 6 years ago

Firefox does it in the url bar. But this is imho a really bad design and clutters it. Maybe the code can be transpanted to the status bar?

	Frank-Rainer Grahl (:frg)
	Comment 8 • 6 years ago

Oops transplanted.
 
If you start work you need to do it in comm-esr60. comm-central is badly broken. For comm-esr60 better pick up some wip patches from here:

http://www.wg9s.com/comm-257/patches/git/comm-esr60/

	Frank-Rainer Grahl (:frg)
	Comment 9 • 6 years ago

That said. Not sure if it is really a good easy first bug. Felix if you need help with building contact me via email.

	Felix Härnström Assignee
	Comment 10 • 6 years ago

Well, I do already have a working solution based on comm-esr52. Just need to add the new icons necessary. I am hoping porting the changes to esr60 should be relatively straight forward.

	Frank-Rainer Grahl (:frg)
	Comment 11 • 6 years ago

Sure happy to test it.

	Felix Härnström Assignee
	Comment 12 • 6 years ago

So, I have experimented a bit with adding zoom +/- buttons to the status bar as suggested. It works well and I like it (just need to add some new icons), but I'm not sure how it should be handled preference-wise. I don't suppose the zoom buttons should be present in the status bar permanently or by default, but there is currently no preference panel for the status bar (and I don't really like adding one just containing a single checkbox for this small feature). Is there somewhere else we could tuck the settings for this feature?

	Frank-Rainer Grahl (:frg)
	Comment 13 • 6 years ago

Well most of todays websites need smaller fonts so I wouldn't mind an unobtrusive small box. 
Maybe [-][<zoom level>%][+] Then it wouldn't even need custom icons.

IanN what do you think?

Felix you might just want to add a wip patch here or upload a screen shot.

	Felix Härnström Assignee
	Comment 15 • 6 years ago

Something like this? I'm going to move my work to comm-esr60 tomorrow or Tuesday at the latest, I can upload a WIP patch then.

	Frank-Rainer Grahl (:frg)
	Comment 16 • 6 years ago

Looks good to me. I meant for the brackets to represent the actual buttons but not bad this way either :) It is a little too wide when a security cert is shown so the word zoom should probably be dropped. The brackages around the actually percentage were also there as placeholder and should go too.

Happy with a esr52 wip patch until we have sorted out the ui.

	bblack
	Comment 17 • 6 years ago

[+] should be on the right and [-] on the left. That's the case with all the programs I know.

	Felix Härnström Assignee
	Comment 18 • 6 years ago

WIP patch based on comm-esr52. Reuses a lot of the event listeners from viewZoomOverlay. I would say it's mostly done in that it does what it's supposed to, but I haven't figured out how to update the zoom value when switching between tabs. As it is, the value is only updated when a page is first loaded. I'm guessing there is some event that I've just overlooked...

	Frank-Rainer Grahl (:frg)
	Comment 19 • 6 years ago

I am a bit swamped right now but will look on it asap. Not that familiar with events but listening to onlocationchange might do the trick. If not the popup blocker and security indicator should already do something similiar here.

	Felix Härnström Assignee
	Comment 20 • 6 years ago

No rush :)

Fixed the issue where the value wasn't updated when switching tabs. Now the zoom value is updated by both onStateChange and onLocationChange in nsBrowserStatusHandler. Not sure if this is the best place to do it (and especially if it might have an impact on performance?), but figured it was a good starting point since that's already where FullZoom.onLocationChange is called.

Also added a toggle to Preferences -> Appearance -> Content. This only toggles visibility of the UI elements for now.

Still based on comm-esr52 though. Will move it to esr60 after feedback.

	Frank-Rainer Grahl (:frg)
	Comment 21 • 6 years ago

Comment on attachment 9014054 [details] [diff] [review]
WIP-670476-statusbarzoom2.patch

suite/browser/navigator.css
With the Modern theme the text is aligned at the bottom and not centered. Should use some addtional css in the theme itself. probably better to move this block into the classic them also.

suite/browser/navigator.js
> var contentPrefs = Services.contentPrefs.QueryInterface(Components.interfaces.nsIContentPrefService2);
Could probably be written as:
> Components.classes["@mozilla.org/content-pref/service;1"]
>  .getService(Components.interfaces.nsIContentPrefService2)
>  .addObserverForName(this.name, this);
See here:
https://hg.mozilla.org/mozilla-central/rev/b50575909c79

Or use the orginal statement with .addObserverForName(this.name, this) and get rid of the var. You decide but use let instead of var otherwise. One less var to carry around.

> var hidden

NIT let

>   switch (aTopic) {
>      case "nsPref:changed":

Only one case here. Not sure if using an if would be less expensive. You decide.

// Don't need to update if same URI
>     if (this.prevURI == aURI)
>       return;
>     
NIT trailing blanks

Zoom is saved by host. Using prePath should avoid any refreshes but you need additonal logic to check the uri. Not sure what is more expensive. This approach here wouldn't work in java :)

> +};
> +
> +
> var g

NIT remove one blank line

> +  gBrowser.addTabsProgressListener(ZoomListeners);
> +
> +
>   window.addEventListener("MozAfterPaint", DelayedStartup, false);

NIT remove one blank line

> +function updateZoomStatus() {

use let instead of var.

suite/browser/navigator.xul

> class="chromeclass-status">   

NIT remove trailing blanks

suite/common/pref/pref-content.xul

We could put this into the next 2.49 but the panel and l10n changes need to go then. For 2.57 this is fine. I would spit the patch into two parts: base and pref panel.

suite/locales/en-US/chrome/common/pref/pref-content.dtd
NIT \ No newline at end of file

That said I like it. Works very well.

	Frank-Rainer Grahl (:frg)
	Comment 22 • 6 years ago

> With the Modern theme the text is aligned at the bottom and not centered.

Small clarification. The button text only.

	Felix Härnström Assignee
	Comment 23 • 6 years ago

New patch based on comm-esr60. Fixed the issues with the last patch, and split it in two. This patch only adds the zoom controls with no localization or pref panel.

I'm also hoping I read the review guidelines correctly - let me know if I should request a review from someone else :)

	Felix Härnström Assignee
	Comment 24 • 6 years ago

Oops, accidentally kept some code for toggling visibility via prefs. Removed.

	Felix Härnström Assignee
	Comment 25 • 6 years ago

And the prefs + localization.

	bblack
	Comment 26 • 6 years ago

I have tested it with SM 2.53. Most looks good.
But if only a single image (jpg, bmp) is displayed, then the zoom display is not updated.

The individual stages are not very even. Sometimes there are cracks in the size of pictures.
With these settings, this looks better with me:
pref("toolkit.zoomManager.zoomValues", "0.25,0.3,0.35,0.4,0.45,0.5,0.58,0.66,0.75,0.87,1,1.15,1.32,1.5,1.75,2,2.3,2.65,3,3.5,4,4.5,5,6");
Maybe we should adapt that.

It might also be helpful if a click on the middle field shows 100% directly again.

	Felix Härnström Assignee
	Comment 27 • 6 years ago

(In reply to bblack from comment #26)
> I have tested it with SM 2.53. Most looks good.
> But if only a single image (jpg, bmp) is displayed, then the zoom display is
> not updated.
> 
> The individual stages are not very even. Sometimes there are cracks in the
> size of pictures.
> With these settings, this looks better with me:
> pref("toolkit.zoomManager.zoomValues",
> "0.25,0.3,0.35,0.4,0.45,0.5,0.58,0.66,0.75,0.87,1,1.15,1.32,1.5,1.75,2,2.3,2.
> 65,3,3.5,4,4.5,5,6");
> Maybe we should adapt that.
> 
> It might also be helpful if a click on the middle field shows 100% directly
> again.

I've attached a patch fixing the image zoom issue and added the zoom reset to the label. Because no zoom prefs are saved for images (to my knowledge) I have to update the zoom label directly in viewZoomOverlay. This seems to be the only straight-forward way of updating the label for images, without changing how the zoom manager and prefs work (which I think would be outside the scope of this issue).

I agree that those zoom increments look nicer, but I'm not sure that should be changed in this issue since it would apply to a lot more than this feature.

	bblack
	Comment 28 • 6 years ago

(In reply to Felix Härnström from comment #27)
> Created attachment 9016924 [details] [diff] [review]
> WIP-670476-statusbarzoomimagefix.patch

> I've attached a patch fixing the image zoom issue and added the zoom reset
> to the label.

That looks very good, thanks.
 
> I agree that those zoom increments look nicer, but I'm not sure that should
> be changed in this issue since it would apply to a lot more than this
> feature.

I think that this is best decided by Ian and Frank.

I do not think you really need this configuration:
pref("browser.zoom.showZoomStatusPanel", false);

I see no disadvantage if it is always active.
Let's see what Frank means.

	Frank-Rainer Grahl (:frg)
	Comment 29 • 6 years ago

Comment on attachment 9016924 [details] [diff] [review]
WIP-670476-statusbarzoomimagefix.patch

Sorry for the late feedback. Still a little bit swamped with paid work and other things.

esr60 no longer uses the Components. syntax. Please switch to Cc. Ci. ...

> suite/themes/classic/navigator/navigator.css
This needs to go into suite/themes/classic/mac/navigator/navigator.css too.

> suite/themes/modern/navigator/navigator.css
> +.zoom-button-align{
> +  padding-bottom: 2px;

3px looks better here. 

If the other zoom levels look better we should fix this in a new bug. The menu needs changing too in this case.

> I do not think you really need this configuration:
> pref("browser.zoom.showZoomStatusPanel", false);

> I see no disadvantage if it is always active.
> Let's see what Frank means.

Well SeaMonkey users are a conservative lot and it might interfere with some themes so let the pref stay. I would add the pref query part back into the first patch because of a possibe inclusion into 2.49.x. Just the tooltip string and the pref panel parts would then be in the seond patch for esr60+.

Still f+ but this should be it then :) Thanks. I still need to ask IanN for a final review or verdict because of the ui change. Will do it in todays status meeting.

	Felix Härnström Assignee
	Comment 30 • 6 years ago

(In reply to Frank-Rainer Grahl (:frg) from comment #29)
> 
> Well SeaMonkey users are a conservative lot and it might interfere with some
> themes so let the pref stay. I would add the pref query part back into the
> first patch because of a possibe inclusion into 2.49.x. Just the tooltip
> string and the pref panel parts would then be in the seond patch for esr60+.

I've attached a patch with the requested changes. So we keep the zoom controls hidden until the preference panel can be added, if I understood you correctly?

	Felix Härnström Assignee
	Comment 31 • 6 years ago

And the pref panel + tooltips.

	Frank-Rainer Grahl (:frg)
	Comment 32 • 6 years ago

Comment on attachment 9016987 [details] [diff] [review]
WIP-670476-statusbarzoomcontrols3.patch

Looks great to me.

> suite/browser/navigator.js	
NIT You missed the Comonents.interfaces. to Ci. here: XPCOMUtils.generateQI([Components.interfaces.n

Can be fixed during checkin. I need a separate ui review. Playing a little with it I am not sure if resetting with a single click is the right choice. If you bring a window to the forground you might accidently click here. Lets see what ianN thinks about it as a whole.

	Frank-Rainer Grahl (:frg)
	Comment 33 • 6 years ago

Comment on attachment 9016988 [details] [diff] [review]
WIP-670476-statusbarzoomprefs2.patch

This is fine anyway. Thanks.

	Frank-Rainer Grahl (:frg)
	Comment 34 • 6 years ago

Local test builds will be available from Bills site tomorrow:

http://www.wg9s.com/comm-esr/

2.57 is already there. 2.53 too should be available tomorrow.

	Ian Neal
	Comment 35 • 6 years ago

Comment on attachment 9016987 [details] [diff] [review]
WIP-670476-statusbarzoomcontrols3.patch

This is a good starting point, it would be useful to have a follow-up bug dealing with adding the same functionality to mail and composer.

	Pulsebot
	Comment 36 • 6 years ago

Pushed by frgrahl@gmx.net:
https://hg.mozilla.org/comm-central/rev/6246b3582096
Add zoom controls to status bar. r=frg
https://hg.mozilla.org/comm-central/rev/67c6b0c3b934
Add preference zoom status panel and tooltips for it. r=frg,IanN

	Frank-Rainer Grahl (:frg)
	Comment 37 • 6 years ago

Comment on attachment 9016987 [details] [diff] [review]
WIP-670476-statusbarzoomcontrols3.patch

IanN should we take this part to 2.49? It works fine there and has no l10n impact.

	Frank-Rainer Grahl (:frg)
	Comment 38 • 6 years ago

Felix, I needed to change the umlauts in you email address. They did show up garbled in the push.

Thanks alot again for the patch.

	Frank-Rainer Grahl (:frg)
	Comment 39 • 6 years ago

Drat. Forgot to fix the nits before the push.

	Pulsebot
	Comment 40 • 6 years ago

Pushed by frgrahl@gmx.net:
https://hg.mozilla.org/comm-central/rev/f3acec1ae9e9
Follow-up. Fix NITs found after pushing the original patch. r=me

	Frank-Rainer Grahl (:frg)
	Comment 41 • 6 years ago

NI for myself to not forget about the followup patches.

	Felix Härnström Assignee
	Comment 42 • 6 years ago

(In reply to Frank-Rainer Grahl (:frg) from comment #38)
> Felix, I needed to change the umlauts in you email address. They did show up
> garbled in the push.
> 
> Thanks alot again for the patch.

Oops, thanks for letting me know. I'll  make sure to change them to their ASCII equivalents. Not the first time my name has managed to trip up a system... :)

	Ian Neal
	Comment 43 • 6 years ago

Comment on attachment 9016987 [details] [diff] [review]
WIP-670476-statusbarzoomcontrols3.patch

a=me

	Ian Neal
	Comment 44 • 6 years ago

Comment on attachment 9016988 [details] [diff] [review]
WIP-670476-statusbarzoomprefs2.patch

a=me

	Ian Neal
	Comment 45 • 6 years ago

Comment on attachment 9020569 [details] [diff] [review]
670476-nits.patch

a=me

	Frank-Rainer Grahl (:frg)
	Comment 46 • 6 years ago

https://hg.mozilla.org/releases/comm-esr60/rev/2839a305d3e62e97613ed50a58c2939733193cc8
Add zoom controls to status bar. r=frg a=IanN

https://hg.mozilla.org/releases/comm-esr60/rev/e3fb99a54a4c8bf0166a1f16f3a55af4ca0c013c
Add preference zoom status panel and tooltips for it. r=frg,IanN a=IanN

https://hg.mozilla.org/releases/comm-esr60/rev/f8590325e3150df1458a0827c701da86b839b807
Follow-up. Fix NITs found after pushing the original patch. r=me a=IanN

	Frank-Rainer Grahl (:frg)
	Comment 47 • 6 years ago

https://hg.mozilla.org/releases/comm-esr52/rev/5879f7bb3e5e9c70d473c71c4ec66a48c58e3d91
Add zoom controls to status bar. r=frg a=IanN