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

Status: RESOLVED FIXED

(SeaMonkey :: UI Design, enhancement)

Comment 1

[Philip Chee]

ping. We can haz patch?

Comment 2

[Serge Gautherie (:sgautherie)]

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]

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)]

Attachment: Port |Bug 352037 - Undo Add To Dictionary| to SeaMonkey (v1)

Comment 5

[[e-mail]]

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

[[e-mail]]

(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)]

Attachment: Port |Bug 352037 - Undo Add To Dictionary| to SeaMonkey (v2)

Comment 8

[[e-mail]]

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)]

Attachment: Port |Bug 352037 - Undo Add To Dictionary| to SeaMonkey (v3)

Comment 10

[Edmund Wong (:ewong)]

Pushed to comm-central:
http://hg.mozilla.org/comm-central/rev/bb6109ec76bf

Comment 11

[Serge Gautherie (:sgautherie)]

Missed to update the related test too:
http://mxr.mozilla.org/comm-central/find?text=&string=test_contextmenu.html

Comment 12

[Edmund Wong (:ewong)]

Attachment: Fixup tests for this bug. (v1)

Comment 13

[Serge Gautherie (:sgautherie)]

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)]

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)]

Attachment: Fixup tests for this bug. (v2)

Comment 16

[Philip Chee]

We don't have the Web Inspector items.
> +                         ].concat(inspectItems));

Comment 17

[Edmund Wong (:ewong)]

Attachment: Fixup tests for this bug. (v3) [Identical to v1]

Comment 18

[Serge Gautherie (:sgautherie)]

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)]

Attachment: Fixup tests for this bug. (v4) [Identical to v2]

Comment 20

[Serge Gautherie (:sgautherie)]

Comment on attachment 639089 [details] [diff] [review]
Fixup tests for this bug. (v4)
[Identical to v2]

See comment 16...

Comment 21

[Edmund Wong (:ewong)]

Attachment: Fixup tests for this bug. (v5)

Comment 22

[Edmund Wong (:ewong)]

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)]

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)]

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)]

(In reply to [e-mail] 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)]

Attachment: Fixed accesskey conflict for Undo Add to Dictionary. (v1)

Comment 27

[Edmund Wong (:ewong)]

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)]

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

[[e-mail]]

[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)]

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]

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)]

(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)]

(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)]

Attachment: Fix tests for test_contextmenu.html (v6)

Comment 35

[Justin Wood (:Callek)]

(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

[[e-mail]]

Comment on attachment 640916 [details] [diff] [review]
Fix tests for test_contextmenu.html (v6)

Seems reasonable to me. Does it work?

Comment 39

[Philip Chee]

Attachment: 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

[[e-mail]]

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]

Pushed test to comm-central:
http://hg.mozilla.org/comm-central/rev/d61f5d649d85