Last Comment Bug 715410 - Port |Bug 352037 - Undo Add To Dictionary| to SeaMonkey
: Port |Bug 352037 - Undo Add To Dictionary| to SeaMonkey
Status: RESOLVED FIXED
[good first bug][mentor=IanN][lang=js...
:
Product: SeaMonkey
Classification: Client Software
Component: UI Design (show other bugs)
: Trunk
: All All
: -- enhancement (vote)
: seamonkey2.12
Assigned To: Edmund Wong (:ewong)
:
Mentors:
: 789002 (view as bug list)
Depends on: 352037 722636
Blocks:
  Show dependency treegraph
 
Reported: 2012-01-04 18:29 PST by Serge Gautherie (:sgautherie)
Modified: 2012-09-25 06:26 PDT (History)
4 users (show)
bugzillamozillaorg_serge_20140323: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
wontfix
wontfix
affected
fixed


Attachments
Port |Bug 352037 - Undo Add To Dictionary| to SeaMonkey (v1) (3.88 KB, patch)
2012-05-27 03:17 PDT, Edmund Wong (:ewong)
neil: review-
Details | Diff | Splinter Review
Port |Bug 352037 - Undo Add To Dictionary| to SeaMonkey (v2) (4.29 KB, patch)
2012-05-27 07:35 PDT, Edmund Wong (:ewong)
neil: review+
Details | Diff | Splinter Review
Port |Bug 352037 - Undo Add To Dictionary| to SeaMonkey (v3) (4.34 KB, patch)
2012-05-28 07:03 PDT, Edmund Wong (:ewong)
ewong: review+
Details | Diff | Splinter Review
Fixup tests for this bug. (v1) (3.03 KB, patch)
2012-06-01 22:49 PDT, Edmund Wong (:ewong)
no flags Details | Diff | Splinter Review
Fixup tests for this bug. (v2) (4.69 KB, patch)
2012-06-02 22:34 PDT, Edmund Wong (:ewong)
no flags Details | Diff | Splinter Review
Fixup tests for this bug. (v3) [Identical to v1] (3.03 KB, patch)
2012-06-04 18:24 PDT, Edmund Wong (:ewong)
no flags Details | Diff | Splinter Review
Fixup tests for this bug. (v4) [Identical to v2] (4.69 KB, patch)
2012-07-04 07:22 PDT, Edmund Wong (:ewong)
no flags Details | Diff | Splinter Review
Fixup tests for this bug. (v5) (8.20 KB, patch)
2012-07-05 01:32 PDT, Edmund Wong (:ewong)
no flags Details | Diff | Splinter Review
Fixed accesskey conflict for Undo Add to Dictionary. (v1) (1.46 KB, patch)
2012-07-05 02:37 PDT, Edmund Wong (:ewong)
neil: review+
philip.chee: approval‑comm‑aurora+
Details | Diff | Splinter Review
Fix tests for test_contextmenu.html (v6) (11.41 KB, patch)
2012-07-10 21:53 PDT, Edmund Wong (:ewong)
no flags Details | Diff | Splinter Review
Fix tests for test_contextmenu.html (v7) (11.45 KB, patch)
2012-09-10 11:31 PDT, Philip Chee
neil: review+
philip.chee: feedback+
Details | Diff | Splinter Review

Description Serge Gautherie (:sgautherie) 2012-01-04 18:29:40 PST

    
Comment 1 Philip Chee 2012-02-25 02:32:23 PST
ping. We can haz patch?
Comment 2 Serge Gautherie (:sgautherie) 2012-02-25 03:24:22 PST
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.
Comment 3 Philip Chee 2012-02-25 10:52:38 PST
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).
Comment 4 Edmund Wong (:ewong) 2012-05-27 03:17:34 PDT
Created attachment 627548 [details] [diff] [review]
Port |Bug 352037 - Undo Add To Dictionary| to SeaMonkey (v1)
Comment 5 neil@parkwaycc.co.uk 2012-05-27 06:14:01 PDT
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.
Comment 6 neil@parkwaycc.co.uk 2012-05-27 06:24:00 PDT
(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"...
Comment 7 Edmund Wong (:ewong) 2012-05-27 07:35:57 PDT
Created attachment 627563 [details] [diff] [review]
Port |Bug 352037 - Undo Add To Dictionary| to SeaMonkey (v2)
Comment 8 neil@parkwaycc.co.uk 2012-05-27 09:24:41 PDT
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();
Comment 9 Edmund Wong (:ewong) 2012-05-28 07:03:33 PDT
Created attachment 627690 [details] [diff] [review]
Port |Bug 352037 - Undo Add To Dictionary| to SeaMonkey (v3)
Comment 10 Edmund Wong (:ewong) 2012-05-28 07:11:26 PDT
Pushed to comm-central:
http://hg.mozilla.org/comm-central/rev/bb6109ec76bf
Comment 11 Serge Gautherie (:sgautherie) 2012-05-29 11:46:01 PDT
Missed to update the related test too:
http://mxr.mozilla.org/comm-central/find?text=&string=test_contextmenu.html
Comment 12 Edmund Wong (:ewong) 2012-06-01 22:49:08 PDT
Created attachment 629440 [details] [diff] [review]
Fixup tests for this bug. (v1)
Comment 13 Serge Gautherie (:sgautherie) 2012-06-02 04:13:08 PDT
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 14 Serge Gautherie (:sgautherie) 2012-06-02 04:20:41 PDT
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"?
Comment 15 Edmund Wong (:ewong) 2012-06-02 22:34:49 PDT
Created attachment 629552 [details] [diff] [review]
Fixup tests for this bug. (v2)
Comment 16 Philip Chee 2012-06-03 09:29:05 PDT
We don't have the Web Inspector items.
> +                         ].concat(inspectItems));
Comment 17 Edmund Wong (:ewong) 2012-06-04 18:24:13 PDT
Created attachment 630027 [details] [diff] [review]
Fixup tests for this bug. (v3)
[Identical to v1]
Comment 18 Serge Gautherie (:sgautherie) 2012-07-04 03:44:19 PDT
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 :-(
Comment 19 Edmund Wong (:ewong) 2012-07-04 07:22:33 PDT
Created attachment 639089 [details] [diff] [review]
Fixup tests for this bug. (v4)
[Identical to v2]
Comment 20 Serge Gautherie (:sgautherie) 2012-07-04 10:44:06 PDT
Comment on attachment 639089 [details] [diff] [review]
Fixup tests for this bug. (v4)
[Identical to v2]

See comment 16...
Comment 21 Edmund Wong (:ewong) 2012-07-05 01:32:12 PDT
Created attachment 639259 [details] [diff] [review]
Fixup tests for this bug. (v5)
Comment 22 Edmund Wong (:ewong) 2012-07-05 01:32:45 PDT
Comment on attachment 639259 [details] [diff] [review]
Fixup tests for this bug. (v5)

Unbitrotted the patch and fixed the nit.
Comment 23 Edmund Wong (:ewong) 2012-07-05 02:02:51 PDT
Comment on attachment 639259 [details] [diff] [review]
Fixup tests for this bug. (v5)

tests don't work.. (oops)

will fix the tests.
Comment 24 Edmund Wong (:ewong) 2012-07-05 02:13:03 PDT
Comment on attachment 639259 [details] [diff] [review]
Fixup tests for this bug. (v5)

submitting this for feedback.
Need some clarifications.
Comment 25 Edmund Wong (:ewong) 2012-07-05 02:22:38 PDT
(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.
Comment 26 Edmund Wong (:ewong) 2012-07-05 02:37:59 PDT
Created attachment 639273 [details] [diff] [review]
Fixed accesskey conflict for Undo Add to Dictionary. (v1)
Comment 27 Edmund Wong (:ewong) 2012-07-05 02:49:06 PDT
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 28 Edmund Wong (:ewong) 2012-07-05 02:49:34 PDT
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:
Comment 29 neil@parkwaycc.co.uk 2012-07-05 02:51:06 PDT
[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 30 Serge Gautherie (:sgautherie) 2012-07-05 02:58:41 PDT
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.
Comment 31 Philip Chee 2012-07-05 02:59:41 PDT
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.
Comment 32 Edmund Wong (:ewong) 2012-07-05 03:03:31 PDT
(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?
Comment 33 Serge Gautherie (:sgautherie) 2012-07-10 10:39:36 PDT
(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...
Comment 34 Edmund Wong (:ewong) 2012-07-10 21:53:57 PDT
Created attachment 640916 [details] [diff] [review]
Fix tests for test_contextmenu.html (v6)
Comment 35 Justin Wood (:Callek) (Away until Aug 29) 2012-07-16 09:50:13 PDT
(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
Comment 36 neil@parkwaycc.co.uk 2012-08-29 09:17:57 PDT
Comment on attachment 640916 [details] [diff] [review]
Fix tests for test_contextmenu.html (v6)

Seems reasonable to me. Does it work?
Comment 37 Phoenix 2012-09-06 12:13:03 PDT
*** Bug 789002 has been marked as a duplicate of this bug. ***
Comment 38 Philippe LEBOURG 2012-09-10 09:30:57 PDT
*** Bug 789002 has been marked as a duplicate of this bug. ***
Comment 39 Philip Chee 2012-09-10 11:31:37 PDT
Created attachment 659794 [details] [diff] [review]
Fix tests for test_contextmenu.html (v7)

> 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
Comment 40 neil@parkwaycc.co.uk 2012-09-11 08:57:45 PDT
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.
Comment 41 Philip Chee 2012-09-25 06:26:03 PDT
Pushed test to comm-central:
http://hg.mozilla.org/comm-central/rev/d61f5d649d85

Note You need to log in before you can comment on or make changes to this bug.