Closed Bug 715410 Opened 13 years ago Closed 12 years ago

Port |Bug 352037 - Undo Add To Dictionary| to SeaMonkey

Categories

(SeaMonkey :: UI Design, enhancement)

Product:
SeaMonkey
Component:
UI Design
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(seamonkey2.9 wontfix, seamonkey2.10 wontfix, seamonkey2.11 affected, seamonkey2.12 fixed)

Status:
RESOLVED FIXED
Milestone:
seamonkey2.12
Tracking Flags:
Tracking Status
seamonkey2.9 --- wontfix
seamonkey2.10 --- wontfix
seamonkey2.11 --- affected
seamonkey2.12 --- fixed

People

(Reporter: sgautherie, Assigned: ewong)

References

Details

(Whiteboard: [good first bug][mentor=IanN][lang=js][lang=xul])

Attachments

(3 files, 8 obsolete files)

Port |Bug 352037 - Undo Add To Dictionary| to SeaMonkey (v3)
(deleted), patch
ewong
: review+
Details | Diff | Splinter Review
Fixed accesskey conflict for Undo Add to Dictionary. (v1)
(deleted), patch
neil
: review+
philip.chee
: approval-comm-aurora+
Details | Diff | Splinter Review
Fix tests for test_contextmenu.html (v7)
(deleted), patch
neil
: review+
philip.chee
: feedback+
Details | Diff | Splinter Review
No description provided.
No longer depends on: 715399
ping. We can haz patch?
Not my priority atm: I assigned me because I was working on test_contextmenu.html, but I then put further work on hold ftb. Feel free to take over.
This might be a GFB so setting the appropriate fields. Whoever does this will have to port Patch v3.1 in Bug 352037 and also the patch in the follow-up Bug 722636 ("Undo Add to Dictionary" context menu is displayed outside of text fields).
Assignee: sgautherie.bz → nobody
Status: ASSIGNED → NEW
Whiteboard: [good first bug][mentor=IanN][lang=js][lang=xul]
status-seamonkey2.9: --- → affected
Depends on: 722636
Target Milestone: seamonkey2.9 → ---
Assignee: nobody → ewong
Status: NEW → ASSIGNED
Attachment #627548 - Flags: review?(neil)
Comment on attachment 627548 [details] [diff] [review] Port |Bug 352037 - Undo Add To Dictionary| to SeaMonkey (v1) When is "Undo Add to Dictionary" supposed to appear? It's currently visible all the time once you add to dictionary, which definitely doesn't make sense when you're not even spellchecking, but also causes an access key conflict when it's visible at the same time as, say, "Add to Dictionary". Also, you have to watch out for when to show the separator.
Attachment #627548 - Flags: review?(neil) → review-
(In reply to comment #5) > When is "Undo Add to Dictionary" supposed to appear? It's currently visible > all the time once you add to dictionary, which definitely doesn't make sense > when you're not even spellchecking, but also causes an access key conflict > when it's visible at the same time as, say, "Add to Dictionary". Also, you > have to watch out for when to show the separator. Looks like porting bug 722636 would have solved most of these, except for the access key conflict, which is caused because we're already using "o" for something else. We don't have any good keys left, which leaves us with "t"...
Attachment #627548 - Attachment is obsolete: true
Attachment #627563 - Flags: review?(neil)
Comment on attachment 627563 [details] [diff] [review] Port |Bug 352037 - Undo Add To Dictionary| to SeaMonkey (v2) >+ var showUndo = canSpell && InlineSpellCheckerUI.canUndo(); Nit: I think I would like this tweaked slightly to use InlineSpellCheckerUI.enabled && InlineSpellCheckerUI.canUndo();
Attachment #627563 - Flags: review?(neil) → review+
Attachment #627563 - Attachment is obsolete: true
Attachment #627690 - Flags: review+
Pushed to comm-central: http://hg.mozilla.org/comm-central/rev/bb6109ec76bf
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Missed to update the related test too: http://mxr.mozilla.org/comm-central/find?text=&string=test_contextmenu.html
Status: RESOLVED → REOPENED
status-seamonkey2.10: --- → affected
status-seamonkey2.11: --- → affected
status-seamonkey2.9: affected → wontfix
Flags: in-testsuite?
Resolution: FIXED → ---
Target Milestone: --- → seamonkey2.12
Status: REOPENED → ASSIGNED
Attached patch Fixup tests for this bug. (v1) (obsolete) (deleted) — — Splinter Review
Attachment #629440 - Flags: review?(iann_bugzilla)
Comment on attachment 629440 [details] [diff] [review] Fixup tests for this bug. (v1) Hum, this patch looks very different to http://hg.mozilla.org/mozilla-central/diff/36b986ef4b5b/browser/base/content/test/test_contextmenu.html Could you explain a little?
Comment on attachment 629440 [details] [diff] [review] Fixup tests for this bug. (v1) Review of attachment 629440 [details] [diff] [review]: ----------------------------------------------------------------- Oh, sorry: I was looking at a later changeset. The right one is: http://hg.mozilla.org/mozilla-central/diff/314f940ca86e/browser/base/content/test/test_contextmenu.html ::: suite/browser/test/mochitest/test_contextmenu.html @@ +19,5 @@ > > /** Test for Login Manager: multiple login autocomplete. **/ > > netscape.security.PrivilegeManager.enablePrivilege('UniversalXPConnect'); > +Components.utils.import("resource://gre/modules/InlineSpellChecker.jsm"); Good. @@ +557,2 @@ > closeContextMenu(); > + openContextMenuFor(textarea, false, true); // Invoke context menu for next test. Good. @@ +616,2 @@ > closeContextMenu(); > + openContextMenuFor(link); Why these, instead of adding the new "case 15"?
Attachment #629440 - Flags: review?(iann_bugzilla) → review?(sgautherie.bz)
Attached patch Fixup tests for this bug. (v2) (obsolete) (deleted) — — Splinter Review
Attachment #629440 - Attachment is obsolete: true
Attachment #629440 - Flags: review?(sgautherie.bz)
Attachment #629552 - Flags: review?(sgautherie.bz)
We don't have the Web Inspector items. > + ].concat(inspectItems));
Attachment #629552 - Attachment is obsolete: true
Attachment #629552 - Flags: review?(sgautherie.bz)
Attachment #630027 - Flags: review?(sgautherie.bz)
Comment on attachment 630027 [details] [diff] [review] Fixup tests for this bug. (v3) [Identical to v1] This v3 patch is just a copy of v1 patch :-(
Attachment #630027 - Attachment description: Fixup tests for this bug. (v3) → Fixup tests for this bug. (v3) [Identical to v1]
Attachment #630027 - Attachment is obsolete: true
Attachment #630027 - Flags: review?(sgautherie.bz)
Attachment #639089 - Flags: review?(sgautherie.bz)
Comment on attachment 639089 [details] [diff] [review] Fixup tests for this bug. (v4) [Identical to v2] See comment 16...
Attachment #639089 - Attachment description: Fixup tests for this bug. (v4) → Fixup tests for this bug. (v4) [Identical to v2]
Attachment #639089 - Attachment is obsolete: true
Attachment #639089 - Flags: review?(sgautherie.bz)
Attached patch Fixup tests for this bug. (v5) (obsolete) (deleted) — — Splinter Review
Attachment #639259 - Flags: review?(sgautherie.bz)
Comment on attachment 639259 [details] [diff] [review] Fixup tests for this bug. (v5) Unbitrotted the patch and fixed the nit.
Comment on attachment 639259 [details] [diff] [review] Fixup tests for this bug. (v5) tests don't work.. (oops) will fix the tests.
Attachment #639259 - Flags: review?(sgautherie.bz)
Attachment #639259 - Attachment is obsolete: true
Comment on attachment 639259 [details] [diff] [review] Fixup tests for this bug. (v5) submitting this for feedback. Need some clarifications.
Attachment #639259 - Attachment is obsolete: false
Attachment #639259 - Flags: feedback?(sgautherie.bz)
(In reply to neil@parkwaycc.co.uk from comment #6) > Looks like porting bug 722636 would have solved most of these, except for > the access key conflict, which is caused because we're already using "o" for > something else. We don't have any good keys left, which leaves us with "t"... Just ran the test. Apparently (and I've tested this) "t" is used by Cut which does appear with "Undo Add To Dictionary", but is greyed out.
Attachment #639273 - Flags: review?(neil)
Attachment #639273 - Flags: review?(neil) → review+
Comment on attachment 639273 [details] [diff] [review] Fixed accesskey conflict for Undo Add to Dictionary. (v1) Pushed to comm-central: http://hg.mozilla.org/comm-central/rev/e53991397f65
Comment on attachment 639273 [details] [diff] [review] Fixed accesskey conflict for Undo Add to Dictionary. (v1) [Approval Request Comment] Regression caused by (bug #): User impact if declined: Testing completed (on m-c, etc.): Risk to taking this patch (and alternatives if risky): String changes made by this patch:
Attachment #639273 - Flags: approval-comm-aurora?
[Approval Request Comment] Regression caused by (bug #): attachment 627690 [details] [diff] [review] User impact if declined: Can't cut from keyboard after adding to dictionary Testing completed (on m-c, etc.): Discovered while writing test Risk to taking this patch (and alternatives if risky): None String changes made by this patch: Access key only
Comment on attachment 639259 [details] [diff] [review] Fixup tests for this bug. (v5) (In reply to Edmund Wong (:ewong) from comment #24) > submitting this for feedback. This seems more like what I expected. > Need some clarifications. Just ask.
Attachment #639259 - Flags: feedback?(sgautherie.bz)
Comment on attachment 639273 [details] [diff] [review] Fixed accesskey conflict for Undo Add to Dictionary. (v1) Neil says: > I think we're allowed access key fixes, since localisers have their own > access keys anyway a=me for comm-aurora.
Attachment #639273 - Flags: approval-comm-aurora? → approval-comm-aurora+
(In reply to Serge Gautherie (:sgautherie) from comment #30) > Comment on attachment 639259 [details] [diff] [review] > Fixup tests for this bug. (v5) > > (In reply to Edmund Wong (:ewong) from comment #24) > > > submitting this for feedback. > > This seems more like what I expected. > > > Need some clarifications. > > Just ask. Pushing this test fix patch, a lot of the tests break.. prior to this patch, the tests are green. What am I missing?
(In reply to Edmund Wong (:ewong) from comment #32) > Pushing this test fix patch, a lot of the tests break.. > prior to this patch, the tests are green. What am I missing? Ftr, local testing passes without the new case 16. Wrt to your patch, it looks like you missed to include bug 722636 test part...
Attached patch Fix tests for test_contextmenu.html (v6) (obsolete) (deleted) — — Splinter Review
Attachment #639259 - Attachment is obsolete: true
Attachment #640916 - Flags: review?(sgautherie.bz)
(In reply to Philip Chee from comment #31) > Comment on attachment 639273 [details] [diff] [review] > Fixed accesskey conflict for Undo Add to Dictionary. (v1) > > a=me for comm-aurora. http://hg.mozilla.org/releases/comm-aurora/rev/3135d932747c
status-seamonkey2.12: affected → fixed
Attachment #640916 - Flags: review?(sgautherie.bz)
Attachment #640916 - Attachment description: Fix tests for test_contexxtmenu.html (v6) → Fix tests for test_contextmenu.html (v6)
Attachment #640916 - Flags: review?(sgautherie.bz)
Attachment #640916 - Flags: review?(sgautherie.bz) → review?(neil)
Comment on attachment 640916 [details] [diff] [review] Fix tests for test_contextmenu.html (v6) Seems reasonable to me. Does it work?
Attachment #640916 - Flags: feedback?(sgautherie.bz)
> Seems reasonable to me. Does it work? 1. Fixed bit rot. 2. Needs to be applied on top of the patch in Bug 789954 otherwise the test will hang until it times out. 3. Results: Passed: 1866 Failed: 0 Todo: 0 Verified that all the tests passed. Skimmed though the patch. Didn't spot anything obviously wrong. f=me
Attachment #640916 - Attachment is obsolete: true
Attachment #640916 - Flags: review?(neil)
Attachment #640916 - Flags: feedback?(sgautherie.bz)
Attachment #659794 - Flags: review?(neil)
Attachment #659794 - Flags: feedback+
Comment on attachment 659794 [details] [diff] [review] Fix tests for test_contextmenu.html (v7) I don't see any bitrot, just a merge conflict i.e. you could have made bug 789954 depend on attachment 640916 [details] [diff] [review] instead.
Attachment #659794 - Flags: review?(neil) → review+
Pushed test to comm-central: http://hg.mozilla.org/comm-central/rev/d61f5d649d85
Status: ASSIGNED → RESOLVED
Closed: 13 years ago → 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: