The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in seamonkey2.12

Status

SeaMonkey
UI Design
--
enhancement
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: sgautherie, Assigned: ewong)

Tracking

Trunk
seamonkey2.12
Dependency tree / graph
Bug Flags:
in-testsuite ?

SeaMonkey Tracking Flags

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

Details

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

Attachments

(3 attachments, 8 obsolete attachments)

4.34 KB, patch
ewong
: review+
Details | Diff | Splinter Review
1.46 KB, patch
neil@parkwaycc.co.uk
: review+
Details | Diff | Splinter Review
11.45 KB, patch
neil@parkwaycc.co.uk
: review+
Philip Chee
: feedback+
Details | Diff | Splinter Review
Comment hidden (empty)

Updated

5 years ago
No longer depends on: 715399

Comment 1

5 years ago
ping. We can haz patch?
(Reporter)

Comment 2

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

5 years ago
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]
(Reporter)

Updated

5 years ago
status-seamonkey2.9: --- → affected
Depends on: 722636
Target Milestone: seamonkey2.9 → ---
(Assignee)

Comment 4

5 years ago
Created attachment 627548 [details] [diff] [review]
Port |Bug 352037 - Undo Add To Dictionary| to SeaMonkey (v1)
Assignee: nobody → ewong
Status: NEW → ASSIGNED
Attachment #627548 - Flags: review?(neil)

Comment 5

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

Comment 6

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

Comment 7

5 years ago
Created attachment 627563 [details] [diff] [review]
Port |Bug 352037 - Undo Add To Dictionary| to SeaMonkey (v2)
Attachment #627548 - Attachment is obsolete: true
Attachment #627563 - Flags: review?(neil)

Comment 8

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

Comment 9

5 years ago
Created attachment 627690 [details] [diff] [review]
Port |Bug 352037 - Undo Add To Dictionary| to SeaMonkey (v3)
Attachment #627563 - Attachment is obsolete: true
Attachment #627690 - Flags: review+
(Assignee)

Comment 10

5 years ago
Pushed to comm-central:
http://hg.mozilla.org/comm-central/rev/bb6109ec76bf
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Reporter)

Comment 11

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

Updated

5 years ago
Status: REOPENED → ASSIGNED
(Assignee)

Comment 12

5 years ago
Created attachment 629440 [details] [diff] [review]
Fixup tests for this bug. (v1)
Attachment #629440 - Flags: review?(iann_bugzilla)
(Reporter)

Comment 13

5 years ago
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?
(Reporter)

Comment 14

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

Comment 15

5 years ago
Created attachment 629552 [details] [diff] [review]
Fixup tests for this bug. (v2)
Attachment #629440 - Attachment is obsolete: true
Attachment #629440 - Flags: review?(sgautherie.bz)
Attachment #629552 - Flags: review?(sgautherie.bz)

Comment 16

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

Comment 17

5 years ago
Created attachment 630027 [details] [diff] [review]
Fixup tests for this bug. (v3)
[Identical to v1]
Attachment #629552 - Attachment is obsolete: true
Attachment #629552 - Flags: review?(sgautherie.bz)
Attachment #630027 - Flags: review?(sgautherie.bz)
(Reporter)

Comment 18

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

Comment 19

5 years ago
Created attachment 639089 [details] [diff] [review]
Fixup tests for this bug. (v4)
[Identical to v2]
Attachment #639089 - Flags: review?(sgautherie.bz)
(Reporter)

Comment 20

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

Comment 21

5 years ago
Created attachment 639259 [details] [diff] [review]
Fixup tests for this bug. (v5)
Attachment #639259 - Flags: review?(sgautherie.bz)
(Assignee)

Comment 22

5 years ago
Comment on attachment 639259 [details] [diff] [review]
Fixup tests for this bug. (v5)

Unbitrotted the patch and fixed the nit.
(Assignee)

Comment 23

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

Updated

5 years ago
Attachment #639259 - Attachment is obsolete: true
(Assignee)

Comment 24

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

Comment 25

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

Comment 26

5 years ago
Created attachment 639273 [details] [diff] [review]
Fixed accesskey conflict for Undo Add to Dictionary. (v1)
Attachment #639273 - Flags: review?(neil)

Updated

5 years ago
Attachment #639273 - Flags: review?(neil) → review+
(Assignee)

Comment 27

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

Comment 28

5 years ago
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
(Reporter)

Comment 30

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

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

Comment 32

5 years ago
(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?
(Reporter)

Updated

5 years ago
status-seamonkey2.10: affected → wontfix
status-seamonkey2.12: --- → affected
(Reporter)

Comment 33

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

Comment 34

5 years ago
Created attachment 640916 [details] [diff] [review]
Fix tests for test_contextmenu.html (v6)
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
(Assignee)

Updated

5 years ago
Attachment #640916 - Flags: review?(sgautherie.bz)
(Assignee)

Updated

5 years ago
Attachment #640916 - Attachment description: Fix tests for test_contexxtmenu.html (v6) → Fix tests for test_contextmenu.html (v6)
Attachment #640916 - Flags: review?(sgautherie.bz)
(Assignee)

Updated

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

Updated

5 years ago
Duplicate of this bug: 789002

Updated

5 years ago
Duplicate of this bug: 789002

Comment 39

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

Comment 41

5 years ago
Pushed test to comm-central:
http://hg.mozilla.org/comm-central/rev/d61f5d649d85
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.