Closed
Bug 337845
Opened 18 years ago
Closed 18 years ago
Spell checking options should be at the bottom of the context menu
Categories
(Firefox :: General, defect)
Tracking
()
RESOLVED
FIXED
Firefox 2 beta2
People
(Reporter: zeniko, Assigned: zeniko)
References
Details
(Keywords: fixed1.8.1)
Attachments
(1 file)
10.37 KB,
patch
|
brettw
:
review+
bugs
:
superreview+
mtschrep
:
approval1.8.1+
|
Details | Diff | Splinter Review |
Since (de)activating the inline spell checker isn't the most often used feature of the context menu and since most text controls (at least under Windows) start with "Undo, Cut, Copy, Paste", I suggest moving the spell checker entries down - below "Select All". Spelling suggestions probably can stay at the top though. Steps to reproduce: 1. Type some text and select it 2. Right-click and click on the fourth menu item (muscle memory for copying) Expected result: The selected text is copied to the clipboard. Actual result: The typed text disappeared (since the fourth item is now Undo).
Assignee | ||
Updated•18 years ago
|
Flags: blocking-firefox2?
Comment 1•18 years ago
|
||
Not a blocker. Here's a mockup of how it should look .. .-----------------------. | few | | foot | | fool | | food | | too | |-----------------------| | Ignore | <-- this will need another bug, I think :) | Add to dictionary | |-----------------------| | Undo | |-----------------------| | Cut | | Copy | | Paste | | Delete | |-----------------------| | Select All | |-----------------------| |xSpell check this field| | Languages >| '-----------------------'
Flags: blocking-firefox2? → blocking-firefox2-
Comment 2•18 years ago
|
||
I'm still a little on the fence on this, design-wise. Most apps with spellchecking only show the options when something is mis-spelled, but that's because spellchecking is either "on" or "off" pretty much globally. We have the "Spellcheck this field" option which seems like a less common user task than "Cut, Copy, Undo, Paste" yet should probably be co-located with the rest of the spelling UI. Need to ruminate for a while. Brett, your thoughts would be appreciated here.
Comment 3•18 years ago
|
||
I think on/off and dictionary selection should be at the bottom. I used to think they should be all together (that's why it's this way now). But I think the context menu if you don't click on a misspelled word is not very good, and some people use it a lot. I think "Ignore" is overkill, and the menu is already getting too big, but we can have that discussion later. I wonder if we can get rid of the divider between the suggestions and "Add to dictionary"/"Ignore". The suggestions are bold, which might provide sufficient differentiation. Then the first section is "thing you can do with this misspelling", while the bottom section is "control spellcheck for this field." I won't get around to it for a while, but it's easy to do at any point in the future. If any newcomer to help, it should be easy. The menu XUL and the code that turns on and off the spellchecking options need to be modified. Contact me for more info.
Whiteboard: [good first bug]
Comment 4•18 years ago
|
||
- Agreed, ignore is for another bug ;) - I'd also played with the "Add to dictionary" in the same section as the spelling terms, would be fine with that, I think.
Assignee | ||
Comment 5•18 years ago
|
||
This patch removes the divider between the search suggestions and "Add to dictionary" and otherwise makes the menus look as suggested by Mike. Brett: Is it intentional that "Add to dictionary" and "Spell check this field" aren't properly capitalized (opposed to e.g. "Select All")?
Attachment #229045 -
Flags: review?(brettw)
Assignee | ||
Updated•18 years ago
|
Status: NEW → ASSIGNED
Whiteboard: [good first bug] → [has patch][needs review brettw]
Target Milestone: --- → Firefox 2 beta2
Updated•18 years ago
|
Blocks: SpellCheckTracking
Comment 6•18 years ago
|
||
Comment on attachment 229045 [details] [diff] [review] fix Looks good. Sorry it took so long.
Attachment #229045 -
Flags: review?(brettw) → review+
Updated•18 years ago
|
Whiteboard: [has patch][needs review brettw] → [has patch]
Assignee | ||
Updated•18 years ago
|
Assignee: nobody → zeniko
Status: ASSIGNED → NEW
Whiteboard: [has patch] → [has patch][checkin needed]
Updated•18 years ago
|
Whiteboard: [has patch][checkin needed] → [has patch][needs sr]
Updated•18 years ago
|
Attachment #229045 -
Flags: superreview?(bugs)
Comment 7•18 years ago
|
||
Comment on attachment 229045 [details] [diff] [review] fix sr=ben@mozilla.org
Attachment #229045 -
Flags: superreview?(bugs) → superreview+
Assignee | ||
Updated•18 years ago
|
Whiteboard: [has patch][needs sr] → [has patch][checkin needed]
Comment 8•18 years ago
|
||
mozilla/browser/base/content/browser-context.inc 1.24 mozilla/browser/base/content/browser.js 1.669 mozilla/toolkit/content/widgets/textbox.xml 1.37
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Whiteboard: [has patch][checkin needed] → [needs approval]
Updated•18 years ago
|
Attachment #229045 -
Flags: approval1.8.1?
Comment 9•18 years ago
|
||
Comment on attachment 229045 [details] [diff] [review] fix approved by schrep for drivers
Attachment #229045 -
Flags: approval1.8.1? → approval1.8.1+
Assignee | ||
Updated•18 years ago
|
Whiteboard: [needs approval] → [checkin needed]
Assignee | ||
Updated•18 years ago
|
Whiteboard: [checkin needed] → [checkin needed (1.8 branch)]
Comment 10•18 years ago
|
||
mozilla/toolkit/content/widgets/textbox.xml 1.21.4.11 mozilla/browser/base/content/browser-context.inc 1.14.2.9 mozilla/browser/base/content/browser.js 1.479.2.177
You need to log in
before you can comment on or make changes to this bug.
Description
•