1284825 - copy a text phrase of a link does not always work

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Fithu]

User Agent: Mozilla/5.0 (Windows NT 6.1; rv:47.0) Gecko/20100101 Firefox/47.0
Build ID: 20160623154057

Steps to reproduce:

Windows 7, Firefox 47.0.1

1. open the Windows- Editor
2. open the Firefox and go to http://reboot.pro/forum/59-imdisk/page-3?prune_day=100&sort_by=Z-A&sort_key=last_post&topicfilter=all. This is a forum with a lot of issues. The issues are links.
3. press Alt to mark in the issue "Keep setting on reinstall" the phrase "settin".
4. press ctrl.+c
5. change to the editor and press Ctrl+V
6. Now in the Editor is the phrase "settin"
7. make the same again with the phrase "mizing" in the issue "Minimizing imdisks"
8. Now in the Editor is still the phrase "settin"



Actual results:

In point 8. the phrase "settin" wa not copied in the clipboard and so I was not able to get it in the editor with Ctrl. + V. There was still the phrase "settin" in the clipboard.


Expected results:

In point 8. should be the text-Phrase "mizing".

-->
I have also tested it with Vivaldi -> There it works as expected.
And Yes, I have tested it in Firefox in savwe mode.

Comment 1

[Alice0775 White]

Regression window: 
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=fbf93932695e&tochange=845f215b717a

Regressed by: eec3c6a55865	Masayuki Nakano — Bug 625151 Reset accesskey state at blur and alt keydown r=enn


STR:
0. Enable Menubar
1. Open any web page
2. Select partial link text with Alt+Mouse dragging
3. Ctrl+C
4. Select partial link text of the other link with Alt+Mouse dragging
5. Ctrl+C

Actual Result:
Copy clipboard at step5 fails.
After step4, you can see the menubar is activated.

Expected Results:
Copy clipboard at step5 should be performed successfully.
After step4, Menubar should not be activated.

Comment 2

[Alice0775 White]

Attachment: sample html ( has 2 links )

Comment 3

[Neil Deakin]

I can't reproduce this with e10s on or off on Windows 7.

Comment 4

[Alice0775 White]

(In reply to Neil Deakin from comment #3)
> I can't reproduce this with e10s on or off on Windows 7.

you should follow STR commen #0 or comment#1

It is 100 % reproducible on windows XP, 8.1 and 10 and maybe 7.

Comment 5

[Alice0775 White]

I correct Step3 and Step5 of comment #1 to more reliable

3. Ctrl+C and then Ctrl+V on editor(or any text field)
5. Ctrl+C and then Ctrl+V on editor(or any text field)

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I reproduced only once after opening the test in new window... It seems that the STR isn't enough to reproduce this bug... (I tested on Win10)

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Ah, perhaps, I need to past in searchbar after Ctrl+C. Then, I reproduce this bug always.

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Looks like that "blur" event handler cancels the cancel of activating menubar of "mousedown". The "blur" event handler wants to listen to deactivating the window but focus move in the window causes this regression. I think that we should ignore some "blur" events which won't inactivate the window or listen to mouseup event to cancel activating menubar at keyup of "Alt" key.

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0ac13b383c7e

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1284825 part.1 Clean up nsMenuBarListener.h and make each specific event handler protected

Review commit: https://reviewboard.mozilla.org/r/63474/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/63474/

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1284825 part.2 nsMenuBarListener should add/remove event listeners by itself

Let's move the code adding/removing event listers to nsMenuBarListener because it makes what we maintain them easier.

This patch makes nsMenuBarListener store event target which is composed document node of the menubar content as a weak reference but this must be safe because when nsMenuBarFrame (stored as mMenuBarFrame) is being destroyed, OnDestroyMenuBarFrame() which clears the storing event target reference is always called.  We should be able to assume that the content and its composed document has never gone before destroying its frame...

Review commit: https://reviewboard.mozilla.org/r/63476/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/63476/

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1284825 part.3 nsMenuBarListener should clear its accesskey state when its top level window is deactivated rather than it receives a blur event

This patch makes nsMenuBarListener clear its accesskey state when it receives a "deactivate" event of its top level window and reverts the change of nsMenuBarListener::Blur() by bug 625151.

"blur" event may be caused by focus move in the contents after "mosuedown" event.  Therefore, mAccessKeyDownCanceled may be cleared unexpectedly.  Listening to "deactive" event keeps bug 625151's fix because it's a bug after deactivating the window with Alt+Tab.

Review commit: https://reviewboard.mozilla.org/r/63478/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/63478/

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Oops, I failed to request review of them to Enn... I'll request with new patches rebased with the latest central.

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0d6d32d6c511

Comment 15

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8769722 [details]
Bug 1284825 part.1 Clean up nsMenuBarListener.h and make each specific event handler protected

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63474/diff/1-2/

Comment 16

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8769723 [details]
Bug 1284825 part.2 nsMenuBarListener should add/remove event listeners by itself

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63476/diff/1-2/

Comment 17

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8769724 [details]
Bug 1284825 part.3 nsMenuBarListener should clear its accesskey state when its top level window is deactivated rather than it receives a blur event

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63478/diff/1-2/

Comment 18

[Neil Deakin]

Comment on attachment 8769724 [details]
Bug 1284825 part.3 nsMenuBarListener should clear its accesskey state when its top level window is deactivated rather than it receives a blur event

https://reviewboard.mozilla.org/r/63478/#review68726

::: layout/xul/nsMenuBarListener.cpp:71
(Diff revision 2)
>    mEventTarget->AddEventListener(NS_LITERAL_STRING("blur"), this, true);
>  
>    mEventTarget->AddEventListener(
>                    NS_LITERAL_STRING("MozDOMFullscreen:Entered"), this, false);
> +
> +  // Needs to listen deactivate event of the window.

listen *to the* deactivate

Comment 19

[Neil Deakin]

Comment on attachment 8769722 [details]
Bug 1284825 part.1 Clean up nsMenuBarListener.h and make each specific event handler protected

https://reviewboard.mozilla.org/r/63474/#review68730

Comment 20

[Neil Deakin]

Comment on attachment 8769722 [details]
Bug 1284825 part.1 Clean up nsMenuBarListener.h and make each specific event handler protected

https://reviewboard.mozilla.org/r/63474/#review68732

Comment 21

[Neil Deakin]

Comment on attachment 8769723 [details]
Bug 1284825 part.2 nsMenuBarListener should add/remove event listeners by itself

https://reviewboard.mozilla.org/r/63476/#review68724

::: layout/xul/nsMenuBarListener.cpp:73
(Diff revision 2)
> +                  NS_LITERAL_STRING("MozDOMFullscreen:Entered"), this, false);
>  }
>  
>  ////////////////////////////////////////////////////////////////////////
>  nsMenuBarListener::~nsMenuBarListener() 
>  {

Maybe we should assert that mEventTarget is null here since OnDestroyMenuBarFrame should have been called.

Comment 22

[Liz Henry (:lizzard) (relman/hg->git project)]

masayuki, are you still working on this or does it just need to land in m-c? Thanks!

Comment 23

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #22)
> masayuki, are you still working on this or does it just need to land in m-c?
> Thanks!

Sorry for the delay, but I have urgent regressions in release build. So, I'll be back after fixing them.

Comment 24

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Masayuki, can you please get these patches landed in Beta50 soon? We seemed to have slipped this another release. I'd be happy to get them in 50.

Comment 25

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Ritu Kothari (:ritu) from comment #24)
> Hi Masayuki, can you please get these patches landed in Beta50 soon? We
> seemed to have slipped this another release. I'd be happy to get them in 50.

Sorry, I'm still working on more urgent bugs which should be landed in early time of a cycle. I'll be back when I have much time to work on this (so, this is very low priority in my queue).

Comment 26

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Not a new regression, marking it as fix-optional based on Masayuki's reply in comment 25.

Comment 27

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=be5a00fb10ce

Comment 28

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8769722 [details]
Bug 1284825 part.1 Clean up nsMenuBarListener.h and make each specific event handler protected

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63474/diff/2-3/

Comment 29

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8769723 [details]
Bug 1284825 part.2 nsMenuBarListener should add/remove event listeners by itself

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63476/diff/2-3/

Comment 30

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8769724 [details]
Bug 1284825 part.3 nsMenuBarListener should clear its accesskey state when its top level window is deactivated rather than it receives a blur event

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63478/diff/2-3/

Comment 31

[Neil Deakin]

Comment on attachment 8769723 [details]
Bug 1284825 part.2 nsMenuBarListener should add/remove event listeners by itself

https://reviewboard.mozilla.org/r/63476/#review101832

Comment 32

[Neil Deakin]

Comment on attachment 8769724 [details]
Bug 1284825 part.3 nsMenuBarListener should clear its accesskey state when its top level window is deactivated rather than it receives a blur event

https://reviewboard.mozilla.org/r/63478/#review101834

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e2102dd028a9
part.1 Clean up nsMenuBarListener.h and make each specific event handler protected r=enndeakin+6102
https://hg.mozilla.org/integration/autoland/rev/54bc467c1f3c
part.2 nsMenuBarListener should add/remove event listeners by itself r=enndeakin+6102
https://hg.mozilla.org/integration/autoland/rev/96089c12d8f9
part.3 nsMenuBarListener should clear its accesskey state when its top level window is deactivated rather than it receives a blur event r=enndeakin+6102

Comment 34

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/e2102dd028a9
https://hg.mozilla.org/mozilla-central/rev/54bc467c1f3c
https://hg.mozilla.org/mozilla-central/rev/96089c12d8f9

Comment 35

[Ryan VanderMeulen [:RyanVM]]

Should we consider this for uplift, Masayuki?

Comment 36

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #35)
> Should we consider this for uplift, Masayuki?

This is very old regression but reported only 6 months ago. So, I think that we don't need to take a risk for this.