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

Status: RESOLVED FIXED

(SeaMonkey :: UI Design, defect)

Description

[rsx11m]

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.

Comment 1

[rsx11m]

Attachment: Reverse order of matching [Checkin: comment 8]

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.

Comment 2

[[e-mail]]

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

Comment 3

[rsx11m]

If not with the loop, how would you correctly assign the "checked" attribute?

Comment 4

[[e-mail]]

Attachment: Possibly Mac-unsafe patch

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.

Comment 5

[rsx11m]

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 6

[[e-mail]]

Comment on attachment 545383 [details] [diff] [review]
Possibly Mac-unsafe patch

Forget that, this doesn't work if something else sets the zoom level.

Comment 7

[rsx11m]

Ok, so attachment 542214 [details] [diff] [review] it is.

Push on trunk please, this needs to land after the patch for bug 667525.

Comment 8

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 542214 [details] [diff] [review]
Reverse order of matching [Checkin: comment 8]

http://hg.mozilla.org/comm-central/rev/c8093f8d23e1