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

RESOLVED FIXED in seamonkey2.5

Status

--
trivial
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: rsx11m.pub, Assigned: rsx11m.pub)

Tracking

Trunk
seamonkey2.5

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

7 years ago
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.
(Assignee)

Comment 1

7 years ago
Created attachment 542214 [details] [diff] [review]
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.
Attachment #542214 - Flags: review?(neil)

Comment 2

7 years ago
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...
(Assignee)

Comment 3

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

Comment 4

7 years ago
Created attachment 545383 [details] [diff] [review]
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.
Attachment #545383 - Flags: review?(stefanh)
(Assignee)

Comment 5

7 years ago
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

7 years ago
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-

Updated

7 years ago
Attachment #542214 - Flags: review?(neil) → review+
(Assignee)

Comment 7

7 years ago
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 → rsx11m.pub
Status: NEW → ASSIGNED
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]

http://hg.mozilla.org/comm-central/rev/c8093f8d23e1
Attachment #542214 - Attachment description: Reverse order of matching → Reverse order of matching [Checkin: comment 8]
Status: ASSIGNED → RESOLVED
Last Resolved: 7 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.