Drag-n-drop url from location bar to a <tab> opens new tab instead of loading url in that tab

VERIFIED FIXED in Firefox 45

Status

()

Firefox
Tabbed Browser
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: arni2033, Assigned: Neil Deakin)

Tracking

({regression})

44 Branch
Firefox 47
regression
Points:
---
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox43 unaffected, firefox44+ wontfix, firefox45+ verified, firefox46+ verified, firefox47 verified)

Details

Attachments

(1 attachment)

(Reporter)

Description

2 years ago
>>>   My Info:   Win7_64, Nightly 46, 32bit, ID 20151223030323
STR:
1. Open http://example.com/ in a new tab
2. Open https://yandex.ru/  in a new tab
3. Switch to tab from Step 1
4. Select all text in urlbar
5. Drag-n-drop it to the tab from Step 2

Result:       
 Url http://example.com/ opened in a new tab

Expectations: 
 Url http://example.com/ should be loaded in the tab from Step 2


Regression range (it's not very precise, but I'm sure bug 1207594 regressed this):
>   pushlog_url: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=9a8f2342fb3116d23989087e026448d38a3768c5&tochange=5430b2dba98b2a39ccdfd3700131f780a27be17c

Comment 1

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=34564c10054d5864e98152b238c9c16b0bad1b80&tochange=90edb8c62dee69fe55faf84507d70570f9d8eaad
status-firefox43: --- → unaffected
status-firefox44: --- → affected
status-firefox45: --- → affected
tracking-firefox44: --- → ?
tracking-firefox45: --- → ?
tracking-firefox46: --- → ?
Version: Trunk → 44 Branch

Comment 2

2 years ago
This seems like a basic scenario, tracked for 44.
tracking-firefox44: ? → +

Comment 3

2 years ago
Neil, based on the regression window in comment 1, would you be able to help investigate and perhaps provide a fix? I think this is a very basic and oft-used scenario that we regressed in 44 and should try fixing it before we release.
Flags: needinfo?(enndeakin)
Just like Ritu said!
tracking-firefox45: ? → +
tracking-firefox46: ? → +

Comment 5

2 years ago
Benjamin, would you consider this a release blocker? I definitely think this is an oft-out scenario that we have regressed in Fx44. Would you be able to help find an owner and possibly a patch? I think this one meets the beta44 uplift criteria "critical (recent) regressions". Thanks!
Flags: needinfo?(benjamin)

Comment 6

2 years ago
Neil, please take a look.

I have no opinion about whether this is a release blocker. The new behavior seems strictly better than the old one, but I'm neither an owner nor even very knowledgeable about how people use drag/drop like this.
Assignee: nobody → enndeakin
Component: Drag and Drop → Tabbed Browser
Flags: needinfo?(benjamin)
Product: Core → Firefox

Comment 7

2 years ago
IMHO, the current behaviour should be restored, because the user gets 2 choices:
#1 opening a new tab by dropping the URL in the tab bar (or between 2 tabs, it's the same)
#2 replacing an existing tab by dropping the URL in the tab

With this bug, the user losts one possibility (#2).
(Assignee)

Comment 8

2 years ago
OK, so the tabbrowser is kind of abusing the copy/move semantics of drag and drop here.

We are dragging a link or plaintext so the 'copy' drag action is correct here. The 'move' drag action would also be ok but the urlbar doesn't support it.

The tabbrowser interprets the 'copy' drag action as 'make a new tab'. That would be correct if the source were another tab, but here the source is text in the url field and we do want to copy the text from there not move it.

I think we probably want to just replace the tab content when dropping text/links on an existing tab and require dropping them between tabs or in the empty area to add a new tab. This is what Chrome and Safari do.

Maybe Dao would know some history of whether this would miss some usecase.
Flags: needinfo?(enndeakin) → needinfo?(dao)
(Assignee)

Comment 9

2 years ago
Created attachment 8713206 [details] [diff] [review]
droptabnocopy

This patch implements what's described above.
Flags: needinfo?(dao)
Attachment #8713206 - Flags: review?(dao)
(In reply to arni2033 from comment #0)
> STR:
> 1. Open http://example.com/ in a new tab
> 2. Open https://yandex.ru/  in a new tab
> 3. Switch to tab from Step 1
> 4. Select all text in urlbar
> 5. Drag-n-drop it to the tab from Step 2
> 
> Result:       
>  Url http://example.com/ opened in a new tab
> 
> Expectations: 
>  Url http://example.com/ should be loaded in the tab from Step 2

I can't seem to reproduce this. I get the expected result when following your steps. Am I missing something?
(Reporter)

Comment 11

2 years ago
(In reply to Dão Gottwald [:dao] from comment #10)
> I get the expected result when following your steps. Am I missing something?
In Step 5, "the tab" means the <tab> element in tabs toolbar, not the web page in tab from Step 2
screencast: https://dl.dropboxusercontent.com/s/xutzba7r8eurgli/bug%201235129%20comment%2011.webm
(In reply to arni2033 from comment #11)
> (In reply to Dão Gottwald [:dao] from comment #10)
> > I get the expected result when following your steps. Am I missing something?
> In Step 5, "the tab" means the <tab> element in tabs toolbar, not the web
> page in tab from Step 2

Yes, I understood that, still can't reproduce. Is this a platform-specific bug? I'm on Linux.
(Assignee)

Comment 13

2 years ago
Looks like Linux doesn't have this issue as it drops with 'link' here. Does reproduce on Windows and Mac though.

Updated

2 years ago
Attachment #8713206 - Flags: review?(dao) → review+

Updated

2 years ago
Keywords: checkin-needed

Comment 14

2 years ago
Neil is on PTO for two weeks. I'm not sure this fits into beta uplift category in any case, but if it does I'll need to find somebody else to do risk assessment and uplifts.  Ritu, thoughts?
Flags: needinfo?(rkothari)
Forwarding the NI to Sylvestre since he is driving Fx45 release.

My thoughts are that we should consider taking it. I stand by my stance that this is a basic scenario that we have regressed. The STR of dragging dropping a URL from location bar into an existing tab worked before and was broken in 44. I wanted to take a fix back then but none was available. For Fx45, if we can get SV/bug opener to validate the fix, it's worth uplifting the patch (one-liner).
Flags: needinfo?(rkothari) → needinfo?(sledru)

Comment 16

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/4d32d1f87b74
Keywords: checkin-needed
(Reporter)

Comment 17

2 years ago
This patch is required to fix another scenario (below).
Summary:  "There's no indication that url will be opened in a new tab"

STR_2:
1. Open new window, open 2 new tab in that window
2. Open   data:text/html,<a href="https://ya.ru">Link</a>   in a new tab in that window
3. Hover mouse over the "Link" on that page, hold left mouse button, move mouse pointer
   to the center of any <tab> in that window.
4. Hold Ctrl key.
5. Release left mouse button.

AR:
 After Step 4 there's no drop indicator (black arrow between tabs)
 After Step 5 https://ya.ru opens in a new tab, near the tab from Step 3.

ER: Either X or Y
 [X] - open new tab
   After Step 4 drop indicator should be shown (to indicate that url will be opened in a new tab)
   After Step 5 https://ya.ru should open in a new tab, near the tab from Step 3.
 [Y] - load url in tab
   After Step 4 there should be no drop indicator
   After Step 5 https://ya.ru should load in the tab from Step 3.

Comment 18

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4d32d1f87b74
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
Gijs, as you know everything about Firefox, do you have an opinion on this? (should we take the patch in 45 or not) Thanks!
Flags: needinfo?(sledru) → needinfo?(gijskruitbosch+bugs)
Comment on attachment 8713206 [details] [diff] [review]
droptabnocopy

Approval Request Comment
[Feature/regressing bug #]: bug 1207594
[User impact if declined]: can't drop urls on tabs anymore to load them in that tab unless you're on linux
[Describe test coverage new/current, TreeHerder]: probably not, both because dnd is "special" for testing, and because this regressed without tests picking it up.
[Risks and why]: one-line change, so the only risk is that that one line could be wrong... The worst that could happen is that we will now open tabs in an existing tab when for some reason the user expected a new tab. I don't see why/how that would ever be the case, though - if you drop a link on a tab, presumably you intend to actually use that tab for the link...
[String/UUID change made/needed]: no.
Flags: needinfo?(gijskruitbosch+bugs)
Attachment #8713206 - Flags: approval-mozilla-beta?
Attachment #8713206 - Flags: approval-mozilla-aurora?
arni2033, could you please verify this issue is fixed as expected on Nightly 2/24 build? Thanks!
Flags: needinfo?(arni2033)

Updated

2 years ago
Flags: qe-verify+
See Also: → bug 1250915
(Reporter)

Comment 22

2 years ago
This bug is fixed on:   Win7_64, Nightly 47, 32bit, ID 20160225030209

Notes:
1) Comment 17 is fixed too:  expectation (Y) is fulfilled
2) Comment 20's "User impact if declined" description is not completely accurate, because I couldn't
   load dragging link in existing tab _only_ if I was dragging url from location bar OR
   was dragging a normal link while holding Ctrl (comment 17). But it's still a good description.
Status: RESOLVED → VERIFIED
Flags: needinfo?(arni2033)
Comment on attachment 8713206 [details] [diff] [review]
droptabnocopy

This has been verified on m-c and sounds pretty low risk. 
Let's take it in aurora and beta.
Attachment #8713206 - Flags: approval-mozilla-beta?
Attachment #8713206 - Flags: approval-mozilla-beta+
Attachment #8713206 - Flags: approval-mozilla-aurora?
Attachment #8713206 - Flags: approval-mozilla-aurora+

Comment 24

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/d0bf1badba3c
status-firefox46: affected → fixed
https://hg.mozilla.org/releases/mozilla-beta/rev/3ee0f5a1619a
status-firefox45: affected → fixed
Verified fixed FX 45 RC, 46.0a2 (2016-03-01) Win 7.
status-firefox45: fixed → verified
status-firefox46: fixed → verified
status-firefox47: fixed → verified
status-firefox44: affected → wontfix
You need to log in before you can comment on or make changes to this bug.