Closed Bug 1284825 Opened 3 years ago Closed 3 years ago

copy a text phrase of a link does not always work

Categories

(Core :: XUL, defect)

13 Branch
x86
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox47 --- wontfix
firefox48 --- wontfix
firefox49 --- wontfix
firefox-esr45 --- wontfix
firefox50 --- wontfix
firefox51 --- wontfix
firefox52 --- wontfix
firefox53 --- fixed

People

(Reporter: Pawihe, Assigned: masayuki)

References

Details

(Keywords: regression, reproducible)

Attachments

(4 files)

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.
Component: Untriaged → General
OS: Unspecified → Windows 7
Hardware: Unspecified → x86
Status: UNCONFIRMED → NEW
Component: General → Untriaged
Ever confirmed: true
Product: Firefox → Core
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.
Blocks: 625151
Component: Untriaged → XP Toolkit/Widgets: Menus
Flags: needinfo?(masayuki)
Flags: needinfo?(enndeakin)
Keywords: regression
I can't reproduce this with e10s on or off on Windows 7.
Flags: needinfo?(enndeakin)
(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.
Keywords: reproducible
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)
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)
Flags: needinfo?(masayuki)
Ah, perhaps, I need to past in searchbar after Ctrl+C. Then, I reproduce this bug always.
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.
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/
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/
Oops, I failed to request review of them to Enn... I'll request with new patches rebased with the latest central.
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
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 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 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
Attachment #8769722 - Flags: review?(enndeakin) → review+
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.
masayuki, are you still working on this or does it just need to land in m-c? Thanks!
Flags: needinfo?(masayuki)
(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.
Flags: needinfo?(masayuki)
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.
Flags: needinfo?(masayuki)
(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).
Flags: needinfo?(masayuki)
Not a new regression, marking it as fix-optional based on Masayuki's reply in comment 25.
Attachment #8769723 - Flags: review?(enndeakin)
Attachment #8769724 - Flags: review?(enndeakin)
Comment on attachment 8769723 [details]
Bug 1284825 part.2 nsMenuBarListener should add/remove event listeners by itself

https://reviewboard.mozilla.org/r/63476/#review101832
Attachment #8769723 - Flags: review?(enndeakin) → review+
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
Attachment #8769724 - Flags: review?(enndeakin) → review+
Pushed by masayuki@d-toybox.com:
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
https://hg.mozilla.org/mozilla-central/rev/e2102dd028a9
https://hg.mozilla.org/mozilla-central/rev/54bc467c1f3c
https://hg.mozilla.org/mozilla-central/rev/96089c12d8f9
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Should we consider this for uplift, Masayuki?
Flags: needinfo?(masayuki)
Version: 47 Branch → 13 Branch
(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.
Flags: needinfo?(masayuki)
QA Whiteboard: [good first verify]
Component: XP Toolkit/Widgets: Menus → XUL
You need to log in before you can comment on or make changes to this bug.