Copying location of a "switch to" action copies moz-action

VERIFIED FIXED

Status

()

VERIFIED FIXED
9 years ago
8 years ago

People

(Reporter: zpao, Assigned: zpao)

Tracking

Trunk
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [switch-to-tab])

Attachments

(1 attachment, 2 obsolete attachments)

You highlight the text in the location bar and it copies the moz-action: at the offset. So you can never actually copy the fill URL (or X characters off the end)
Probably want to add some _parseActionUrl magic to _getSelectedValueForClipboard.
Posted patch Patch v0.1 (WIP) (obsolete) — Splinter Review
(In reply to comment #1)
> Probably want to add some _parseActionUrl magic to
> _getSelectedValueForClipboard.

Yea I saw that. Alternatively (in other words what I've done so far) you can use the actual inputField.

The only "tricky" thing is when you cut, depending on how you do it, the action moz-action stays and you can still switch tabs or it wipes out the moz-action stuff. I'm  thinking the 2nd is better (and that's how I did it).

Note: I don't think I had to change all of the urlbar.selectionStart to urlbar.inputField.selectionStart, but it looks more consistent.
Comment on attachment 436136 [details] [diff] [review]
Patch v0.1 (WIP)

>diff --git a/browser/base/content/urlbarBindings.xml b/browser/base/content/urlbarBindings.xml

>       <method name="_getSelectedValueForClipboard">

>-          var val = this.value.substring(this.selectionStart, this.selectionEnd);
>+          // Grab the actual input field's value, not our value, which could include moz-action:
>+          var val = this.inputField.value.substring(this.inputField.selectionStart, this.inputField.selectionEnd);

I would avoid the changes for selectionStart/selectionEnd, I don't think there's any real clarity benefit to getting them from .inputField just because that's where you also get the value.

>           // If the entire value is selected and it's a valid non-javascript,
>           // non-data URI, encode it.
>           if (val == this.value &&

Seems like this should use inputField.value too. Probably worth putting it into a temporary.
Posted patch Patch v0.2 (obsolete) — Splinter Review
Now with a test and less inputField.selectionStart/End
Assignee: nobody → paul
Attachment #436136 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #437482 - Flags: review?(gavin.sharp)
Comment on attachment 437482 [details] [diff] [review]
Patch v0.2

Worth testing this test on linux, selection clipboard might cause issues there.

I think you need to change the test to poll the clipboard to avoid random oranges (see e.g. browser_410196_paste_into_tags.js and browser_bug321000.js) - r- because of that.

Patch looks good otherwise, though I'd probably use a temporary for this.inputField.value in _getSelectionForClipboard.
Attachment #437482 - Flags: review?(gavin.sharp) → review-
This is a bad regression... Requesting blocking for 1.9.3.
blocking2.0: --- → ?
status2.0: --- → ?
Posted patch Patch v0.3Splinter Review
Now with polling tests & used a temp var to save a value lookup. I'll push to try to see how Linux goes.
Attachment #437482 - Attachment is obsolete: true
Attachment #445172 - Flags: review?
Attachment #445172 - Flags: review? → review?(gavin.sharp)
just passing,
+              // This should reset any "moz-action;" prefix.

I guess you wanted to put a colon there and not a semicolon.

PS: I love the fact you wait for clipboard, well done!
Attachment #445172 - Flags: review?(gavin.sharp) → review+
Pushed http://hg.mozilla.org/mozilla-central/rev/3ae424ac844a (with comment fix).
Status: ASSIGNED → RESOLVED
blocking2.0: ? → ---
Last Resolved: 9 years ago
status2.0: ? → ---
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [switch-to-tab]
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.