Closed Bug 1565273 Opened 6 months ago Closed 6 months ago

Right click Address bar does not select whole url

Categories

(Firefox :: Address Bar, defect, P1)

70 Branch
Desktop
All
defect
Points:
3

Tracking

()

VERIFIED FIXED
Firefox 70
Iteration:
70.2 - Jul 22 - Aug 4
Tracking Status
firefox-esr60 --- unaffected
firefox-esr68 --- unaffected
firefox68 --- unaffected
firefox69 + verified
firefox70 + verified

People

(Reporter: alice0775, Assigned: harry)

References

(Blocks 1 open bug, Regression)

Details

(Keywords: nightly-community, regression)

Attachments

(3 files, 1 obsolete file)

Reproducible: always

Steps To Reproduce:

  1. Open any web page
  2. Wait for page loading
  3. Right click Address bar

Actual Results:
Caret blinks at beginning of url. And context Menu Pops Up

Expected Results:
Whole url should be selected. And context Menu Pops Up

Regression window:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=9381b434999bc6f36111d1d307416a1f6d57c84c&tochange=759f043488918654501e945736c825529ea70fa5

Regressed by: 759f043488918654501e945736c825529ea70fa5 harry — Bug 1562145 - Fix mouse-dragging regression caused by patch to 1554864. r=dao

harry,
Your patch seems to cause the regression. Can you please look into this?

Flags: needinfo?(htwyford)

please, let's add simple mochitest-browser for these basic selection/selectall actions.

Version: 69 Branch → 70 Branch

Sorry to only post a WIP patch for this, but I'm out of office tomorrow. I think this issue only affects Windows, and I've removed the part that would cause the issue. Unfortunately I don't have access to a Windows machine or a VM atm-- maybe someone with one could take a look and see if this patch works and if it causes further issues.

This patch also includes tests, but they're WIP and some of them fail/hang the browser. Dao, if you have time, would you mind taking a look to see if there's something obviously wrong with the tests, esp the right-click test? That's the one that hangs. I haven't written too many of these so it might be a beginner's mistake.

If we want to get the fix out quickly so we can uplift bug 1562145 ASAP, someone could post the fix part tomorrow (provided it works...). Otherwise, I could read feedback on the tests and work on them on the weekend/Monday.

Flags: needinfo?(htwyford) → needinfo?(dao+bmo)
Assignee: nobody → htwyford
Priority: -- → P1
Points: --- → 3
Status: NEW → ASSIGNED
Attachment #9077544 - Attachment description: Bug 1565273 - WIP - Allow context menu clicks to select all on Windows. → Bug 1565273 - Allow context menu clicks to select all on Windows and add tests for selectAll behaviour. r?mak
Iteration: --- → 70.1 - Jul 8 - 21
Flags: needinfo?(dao+bmo)

(In reply to Harry Twyford [:harry] from comment #3)

I think this issue only affects Windows, and I've removed the part that would cause the issue. Unfortunately I don't have access to a Windows machine or a VM atm-- maybe someone with one could take a look and see if this patch works and if it causes further issues.

This is outdated, right? The bug affects all the cases where clickSelectsAll is true, I didn't test on Mac the before/after situation though. On linux clickSelectsAll is false, but one can set it to true, and afaict it will work as on Windows.
And if I read the patch correctly, we are indeed not making a distinction anymore. Is this correct?

This bug doesn't affect me on my Mac. I think the bad behaviour is caused by the Windows platform gate (removed in the attached patch), meaning this issue only affects Windows. This bug is independent of the browser.urlbar.clickSelectsAll pref and whether clickSelectsAll happens at all -- it only affects contextmenu clicks, and only the contextmenu handler has the Windows platform gate.

The Windows platform gate originates here: https://searchfox.org/mozilla-central/source/toolkit/content/widgets/textbox.xml#248. I ported it over to the UrlbarInput contextmenu handler when I was removing the clickSelectsAll behaviour from XUL textboxes. This bug shows that the platform gate now causes issues -- I believe Windows was getting its contextmenu clickSelectsAll behaviour from somewhere else in textbox.xml, and that is now removed.

(In reply to Harry Twyford [:harry] from comment #5)

This bug is independent of the browser.urlbar.clickSelectsAll pref and whether clickSelectsAll happens at all

With version 68, on Linux if I set clickSelectsAll to false, it doesn't select on contextmenu. It does with CSA set to true.
On Windows it does regardless of CSA.
With your patch, on all platforms it acts like on Linux, that is not the old behavior. I think the old Windows behavior makes more sense for all the platforms, on right click we should select all regardless of clickSelectsAll.

Attachment #9077544 - Attachment description: Bug 1565273 - Allow context menu clicks to select all on Windows and add tests for selectAll behaviour. r?mak → Bug 1565273 - Context menu clicks ignore browser.urlbar.clickSelectsAll pref. Add tests for selectAll behaviour. r?mak

FWIW, on GNOME Web, the behavior is what exists currently in Firefox (at least on Linux). Same with Falkon.

Chromium does the previous Windows behavior.

Safari doesn't place a cursor or perform any selection, it just shows a context menu.

I'm not sure that the GNOME Web behavior is desirable (or even designed behavior), but figured I would note it, since I am generally in favor of good platform integration.

Iteration: 70.1 - Jul 8 - 21 → 70.2 - Jul 22 - Aug 4
Pushed by htwyford@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/89925bf4fa18
Context menu clicks ignore browser.urlbar.clickSelectsAll pref. Add tests for selectAll behaviour. r=mak
Duplicate of this bug: 1569473
Has Regression Range: --- → yes
Has STR: --- → yes
OS: Windows 10 → All
Flags: needinfo?(htwyford)
Pushed by htwyford@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d6d95a998ee9
Context menu clicks ignore browser.urlbar.clickSelectsAll pref. Add tests for selectAll behaviour. r=mak
Regressions: 1569813

This was backed out because of frequent failures of Bug 1569813.

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&revision=d6d95a998ee92e29376666d918c6b52dfd651917&selectedJob=258879611
https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&selectedJob=258896828&resultStatus=testfailed%2Cbusted%2Cexception&revision=8419292a5c75752f661b0ca7f1d2dde341faa8ce&searchStr=os%2Cx%2C10.14%2Cshippable%2Copt%2Cmochitests%2Ctest-macosx1014-64-shippable%2Fopt-mochitest-browser-chrome-e10s-2%2Cm%28bc2%29

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=258896828&repo=autoland&lineNumber=4034
[task 2019-07-29T23:46:06.588Z] 23:46:06 INFO - TEST-OK | browser/components/urlbar/tests/browser/browser_urlbar_selection.js | took 318ms
[task 2019-07-29T23:46:06.600Z] 23:46:06 INFO - checking window state
[task 2019-07-29T23:46:06.611Z] 23:46:06 ERROR - GECKO(1882) | TEST-UNEXPECTED-FAIL | unknown test url | uncaught exception - NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIController.doCommand] at doCommand@chrome://global/content/elements/textbox.js:221:18
[task 2019-07-29T23:46:06.612Z] 23:46:06 INFO - GECKO(1882) | _initUI/<@chrome://global/content/elements/textbox.js:110:16
[task 2019-07-29T23:46:06.612Z] 23:46:06 INFO - GECKO(1882) | EventListener.handleEvent*_initUI@chrome://global/content/elements/textbox.js:107:22
[task 2019-07-29T23:46:06.612Z] 23:46:06 INFO - GECKO(1882) | connectedCallback@chrome://global/content/elements/textbox.js:73:12
[task 2019-07-29T23:46:06.612Z] 23:46:06 INFO - GECKO(1882) | LifecycleConnectedCallback*@chrome://global/content/elements/textbox.js:232:18
[task 2019-07-29T23:46:06.612Z] 23:46:06 INFO - GECKO(1882) | @chrome://global/content/customElements.js:727:29
[task 2019-07-29T23:46:06.612Z] 23:46:06 INFO - GECKO(1882) | @chrome://global/content/customElements.js:746:3
[task 2019-07-29T23:46:06.612Z] 23:46:06 INFO - GECKO(1882) | observe@resource://gre/modules/CustomElementsListener.jsm:25:31
[task 2019-07-29T23:46:06.612Z] 23:46:06 INFO - GECKO(1882) | JavaScript error: chrome://global/content/elements/textbox.js, line 221: NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIController.doCommand]
[task 2019-07-29T23:46:06.612Z] 23:46:06 INFO - GECKO(1882) | JavaScript error: resource://testing-common/PromiseTestUtils.jsm, line 112: uncaught exception: Object

Flags: needinfo?(htwyford)
Backout by csabou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e1464f88d798
Backed out changeset d6d95a998ee9 for causing bug 1569813.
Blocks: 1570474

These test failures on Mac look like they might be fairly involved to solve, so on mak's recommendation I've disabled the new tests on Mac and filed follow-up bug 1570474. A new revision with the tests disabled on Mac is now on Phabricator. Here's try with browser_urlbar_selection.js re-run a number of times on Windows and Linux to check for intermittent failure: https://treeherder.mozilla.org/#/jobs?repo=try&revision=76e992ee84aa9e9d2412e8ebe67e8dda6b49e72f

Flags: needinfo?(htwyford)
Pushed by htwyford@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0ab19ed23861
Context menu clicks ignore browser.urlbar.clickSelectsAll pref. Add tests for selectAll behaviour. r=mak
Status: ASSIGNED → RESOLVED
Closed: 6 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 70

Harry, could you please ask for Beta uplift?

Flags: needinfo?(htwyford)

Comment on attachment 9077544 [details]
Bug 1565273 - Context menu clicks ignore browser.urlbar.clickSelectsAll pref. Add tests for selectAll behaviour. r?mak

Beta/Release Uplift Approval Request

  • User impact if declined: Annoying user interface regression.
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): The change to user interface behaviour is very small: just a couple of lines. It affects only text-selection behaviour, so the worst-case outcome is just some other similar text-selection regression. The vast majority of this patch is simply new tests, for both the new and existing text-selection behaviour.
  • String changes made/needed:
Flags: needinfo?(htwyford)
Attachment #9077544 - Flags: approval-mozilla-beta?

I think we should have QA do some testing around this since this is itself a regression from another patch which landed in this code.

Flags: qe-verify+
Flags: in-testsuite+

Comment on attachment 9077544 [details]
Bug 1565273 - Context menu clicks ignore browser.urlbar.clickSelectsAll pref. Add tests for selectAll behaviour. r?mak

Fixes a regression in URL bar behavior in Fx69. Approved for 69.0b11.

Attachment #9077544 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Ah! Is it too late to retract beta approval? I just realized there are a couple of nits in this patch that I didn't fix. I'm going to fix them and post a new patch shortly.

Flags: needinfo?(ryanvm)

Comment on attachment 9077544 [details]
Bug 1565273 - Context menu clicks ignore browser.urlbar.clickSelectsAll pref. Add tests for selectAll behaviour. r?mak

No worries. Just attach a rolled-up patch for Beta uplift when you're ready to do so and re-request approval (you don't need to fill out the questionnaire again, just setting the flag is enough).

Flags: needinfo?(ryanvm)
Attachment #9077544 - Flags: approval-mozilla-beta+
Attached patch bug_1565273_beta.diff (obsolete) — Splinter Review

Beta/Release Uplift Approval Request

  • User impact if declined:
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: Yes
  • If yes, steps to reproduce:
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky):
  • String changes made/needed:
Attachment #9082691 - Flags: approval-mozilla-beta?
Attachment #9077544 - Flags: approval-mozilla-beta?

I removed two console.log statements that snuck their way into the previous patch. I'll prepare a patch removing those statements from Nightly as well.

Attachment #9077544 - Flags: approval-mozilla-beta?
Comment on attachment 9082691 [details] [diff] [review]
bug_1565273_beta.diff

Thanks.
Attachment #9082691 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Backed out changeset 209780edbdf9 (Bug 1565273) for browser-chrome failures at browser/components/urlbar/tests/browser/browser_urlbar_selection.js

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=mozilla-beta&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&revision=209780edbdf9b660eb86fc50dea6041b3a4f977f

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=259797469&repo=mozilla-beta&lineNumber=4003

Backout link: https://treeherder.mozilla.org/#/jobs?repo=mozilla-beta&selectedJob=259797469&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&revision=1270a56d8bb7f6df12008e24b0e56d3b7cf9a160

[task 2019-08-03T20:50:47.777Z] 20:50:47     INFO - TEST-PASS | browser/components/urlbar/tests/browser/browser_urlbar_selection.js | The entire URL should be selected. - 21 == 21 - 
[task 2019-08-03T20:50:47.777Z] 20:50:47     INFO - Buffered messages finished
[task 2019-08-03T20:50:47.778Z] 20:50:47     INFO - TEST-UNEXPECTED-FAIL | browser/components/urlbar/tests/browser/browser_urlbar_selection.js | Uncaught exception - at chrome://mochitests/content/browser/browser/components/urlbar/tests/browser/browser_urlbar_selection.js:150 - TypeError: gURLBar.querySelector is not a function
[task 2019-08-03T20:50:47.778Z] 20:50:47     INFO - Stack trace:
[task 2019-08-03T20:50:47.778Z] 20:50:47     INFO - rightClickSelectsAll@chrome://mochitests/content/browser/browser/components/urlbar/tests/browser/browser_urlbar_selection.js:150:29
[task 2019-08-03T20:50:47.782Z] 20:50:47     INFO - Leaving test bound rightClickSelectsAll
[task 2019-08-03T20:50:47.782Z] 20:50:47     INFO - Entering test bound contextMenuDoesNotCancelSelection
[task 2019-08-03T20:50:47.783Z] 20:50:47     INFO - TEST-PASS | browser/components/urlbar/tests/browser/browser_urlbar_selection.js | The selection should not have changed. - 3 == 3 - 
[task 2019-08-03T20:50:47.783Z] 20:50:47     INFO - TEST-PASS | browser/components/urlbar/tests/browser/browser_urlbar_selection.js | The selection should not have changed. - 7 == 7 - 
[task 2019-08-03T20:50:47.784Z] 20:50:47     INFO - Not taking screenshot here: see the one that was previously logged
[task 2019-08-03T20:50:47.784Z] 20:50:47     INFO - TEST-UNEXPECTED-FAIL | browser/components/urlbar/tests/browser/browser_urlbar_selection.js | Uncaught exception - at chrome://mochitests/content/browser/browser/components/urlbar/tests/browser/browser_urlbar_selection.js:210 - TypeError: gURLBar.querySelector is not a function
[task 2019-08-03T20:50:47.784Z] 20:50:47     INFO - Stack trace:
[task 2019-08-03T20:50:47.785Z] 20:50:47     INFO - contextMenuDoesNotCancelSelection@chrome://mochitests/content/browser/browser/components/urlbar/tests/browser/browser_urlbar_selection.js:210:11
[task 2019-08-03T20:50:47.789Z] 20:50:47     INFO - Leaving test bound contextMenuDoesNotCancelSelection
[task 2019-08-03T20:50:47.793Z] 20:50:47     INFO - Entering test bound dragSelect
Flags: needinfo?(htwyford)
QA Whiteboard: [qa-triaged]
Duplicate of this bug: 1571601

I tested this issue on Windows 10, and Ubuntu 18.04, using the Nightly build from July 30 (build id 20190730095207) where the issue is reproducible, and I tested it on the latest Nightly from August 5th (build id 20190805095413) where the issue is fixed. Note that I also checked on Mac 10.14 using the latest Nightly, and issue is not reproducible. (based on comment 5)

Based on this, I can confirm the issue is fixed.

I will also verify this on beta when the fix lands.

Pushed by htwyford@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d1b00f87871a
Remove unnecessary console.logs. r=mak

Here's the updated beta diff. The old one was backed out since I used gURLBar.querySelector to fetch the contextmenu, but this behaviour was recently changed. The attached diff uses document.getAnonymousElementByAttribute which works in beta. Here's a green beta try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=cfd9849d1ac79fe07a7ea799d5684c34f0c4aa26&selectedJob=260118746 (there's an opt build failure on Linux -- looks like it's an intermittent? I don't see the specific change in this patch causing a build failure).

Because of the previous backouts, Mak could you please take one last look at this patch before I request beta uplift?

Attachment #9082691 - Attachment is obsolete: true
Flags: needinfo?(htwyford) → needinfo?(mak77)

Comment on attachment 9083353 [details] [diff] [review]
bug_1565273_beta_updated.diff

Beta/Release Uplift Approval Request

  • User impact if declined:
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: Yes
  • If yes, steps to reproduce:
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky):
  • String changes made/needed:
Attachment #9083353 - Flags: approval-mozilla-beta?

Mak messaged me that he's having issues posting on Bugzilla today, but that the patch looks good. I requested uplift -- please see filled-out uplift request in comment 19.

Flags: needinfo?(mak77)
Comment on attachment 9083353 [details] [diff] [review]
bug_1565273_beta_updated.diff

Fixes a regression in URL bar behavior in Fx69, patch has tests. Approved for 69.0b12, thanks.
Attachment #9083353 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Hi,

I tested this issue on Windows 10 and Ubuntu 18.04, using the Beta version 69.0b12 where the issue is indeed fixed. Given that it's fixed on Nightly and Beta, I changed the status of the bug to verified fixed.

Best regards, Flor.

Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.