Closed
Bug 1284825
Opened 8 years ago
Closed 8 years ago
copy a text phrase of a link does not always work
Categories
(Core :: XUL, defect)
Tracking
()
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
Updated•8 years ago
|
Status: UNCONFIRMED → NEW
Component: General → Untriaged
Ever confirmed: true
Product: Firefox → Core
Comment 1•8 years ago
|
||
str |
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
status-firefox47:
--- → wontfix
status-firefox48:
--- → wontfix
status-firefox49:
--- → affected
status-firefox50:
--- → affected
status-firefox-esr45:
--- → wontfix
Component: Untriaged → XP Toolkit/Widgets: Menus
Flags: needinfo?(masayuki)
Flags: needinfo?(enndeakin)
Keywords: regression
Comment 2•8 years ago
|
||
Comment 3•8 years ago
|
||
I can't reproduce this with e10s on or off on Windows 7.
Flags: needinfo?(enndeakin)
Comment 4•8 years ago
|
||
(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
Comment 5•8 years ago
|
||
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)
Assignee | ||
Comment 6•8 years ago
|
||
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)
Assignee | ||
Comment 7•8 years ago
|
||
Ah, perhaps, I need to past in searchbar after Ctrl+C. Then, I reproduce this bug always.
Assignee | ||
Comment 8•8 years ago
|
||
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.
Assignee | ||
Comment 9•8 years ago
|
||
Assignee | ||
Comment 10•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/63474/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/63474/
Assignee | ||
Comment 11•8 years ago
|
||
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/
Assignee | ||
Comment 12•8 years ago
|
||
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/
Assignee | ||
Comment 13•8 years ago
|
||
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
Assignee | ||
Comment 14•8 years ago
|
||
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 18•8 years ago
|
||
mozreview-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/#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•8 years ago
|
||
mozreview-review |
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•8 years ago
|
||
mozreview-review |
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 21•8 years ago
|
||
mozreview-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!
status-firefox51:
--- → affected
Flags: needinfo?(masayuki)
Assignee | ||
Comment 23•8 years ago
|
||
(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)
Assignee | ||
Comment 25•8 years ago
|
||
(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.
Updated•8 years ago
|
Attachment #8769723 -
Flags: review?(enndeakin)
Updated•8 years ago
|
Attachment #8769724 -
Flags: review?(enndeakin)
Assignee | ||
Comment 27•8 years ago
|
||
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 31•8 years ago
|
||
mozreview-review |
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 32•8 years ago
|
||
mozreview-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+
Comment 33•8 years ago
|
||
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
Comment 34•8 years ago
|
||
bugherder |
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: 8 years ago
status-firefox53:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Comment 35•8 years ago
|
||
Should we consider this for uplift, Masayuki?
Assignee | ||
Comment 36•8 years ago
|
||
(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)
Updated•8 years ago
|
Updated•8 years ago
|
QA Whiteboard: [good first verify]
Updated•6 years ago
|
Component: XP Toolkit/Widgets: Menus → XUL
You need to log in
before you can comment on or make changes to this bug.
Description
•