Closed Bug 667529 Opened 13 years ago Closed 13 years ago

Don't use the "Other" entry in View > Zoom if a matching menu item is available


(SeaMonkey :: UI Design, defect)

Not set


(Not tracked)



(Reporter:, Assigned:



(1 file, 1 obsolete file)

Following observation when testing bug 621823 (without the need of any of those patches applied): Select an "Other" value that corresponds to one of the fixed values in the menu (e.g., 150%). Then, use Ctrl +/- to browse through the levels and have a look at View > Zoom each time.

Observed behavior: When reaching the 150%, it's shown selected as "Other".
Expected behavior: The pre-defined 150% entry should be selected.

This would also direct the user to the existence of a level in the list which he or she typed in as the "Other" value.
That's the simplest solution to fix this I could come up with. Just reversing the order of traversal of the menu entries shows an existing pre-defined zoom level if such exists, and "Other" only if it is not contained in the list.

I don't think it's worth the effort trying to actually distinguish between a value selected from the menu vs. one that was explicitly entered using the "Other" option, as this would require some persistent variable in addition to the selected value which would need to be reset when Ctrl +/- is used, so this patch appears to be the most attractive and concise solution.
Attachment #542214 - Flags: review?(neil)
Aha, the reason this wasn't previously a problem is that the "Other" entry used to be prohibited from matching an existing menuitem.

I wonder whether we still need a loop. I'd need someone to test on a Mac...
If not with the loop, how would you correctly assign the "checked" attribute?
Attached patch Possibly Mac-unsafe patch (obsolete) — Splinter Review
While you're there it would be handy if you could verify the bug and the first patch on the Mac just so that we know where we are if this patch doesn't work.
Attachment #545383 - Flags: review?(stefanh)
Comment on attachment 545383 [details] [diff] [review]
Possibly Mac-unsafe patch

>+  popup.getElementsByAttribute("value", Math.round(ZoomManager.zoom * 100))[0]
>+       .setAttribute("checked", "true");

I see, that sure answers my previous question. ;-)
Comment on attachment 545383 [details] [diff] [review]
Possibly Mac-unsafe patch

Forget that, this doesn't work if something else sets the zoom level.
Attachment #545383 - Flags: review?(stefanh) → review-
Attachment #542214 - Flags: review?(neil) → review+
Ok, so attachment 542214 [details] [diff] [review] it is.

Push on trunk please, this needs to land after the patch for bug 667525.
Assignee: nobody →
Keywords: checkin-needed
Whiteboard: [c-n: comment #7]
Attachment #545383 - Attachment is obsolete: true
Comment on attachment 542214 [details] [diff] [review]
Reverse order of matching [Checkin: comment 8]
Attachment #542214 - Attachment description: Reverse order of matching → Reverse order of matching [Checkin: comment 8]
Closed: 13 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [c-n: comment #7]
Target Milestone: --- → seamonkey2.5
You need to log in before you can comment on or make changes to this bug.