Closed
Bug 1235129
Opened 8 years ago
Closed 8 years ago
Drag-n-drop url from location bar to a <tab> opens new tab instead of loading url in that tab
Categories
(Firefox :: Tabbed Browser, defect)
Tracking
()
VERIFIED
FIXED
Firefox 47
People
(Reporter: arni2033, Assigned: enndeakin)
References
Details
(Keywords: regression)
Attachments
(1 file)
1007 bytes,
patch
|
dao
:
review+
lizzard
:
approval-mozilla-aurora+
lizzard
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
>>> 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
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
This seems like a basic scenario, tracked for 44.
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)
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•8 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
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•8 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•8 years ago
|
||
This patch implements what's described above.
Flags: needinfo?(dao)
Attachment #8713206 -
Flags: review?(dao)
Comment 10•8 years ago
|
||
(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•8 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
Comment 12•8 years ago
|
||
(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•8 years ago
|
||
Looks like Linux doesn't have this issue as it drops with 'link' here. Does reproduce on Windows and Mac though.
Updated•8 years ago
|
Attachment #8713206 -
Flags: review?(dao) → review+
Updated•8 years ago
|
Keywords: checkin-needed
Comment 14•8 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•8 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/4d32d1f87b74
Keywords: checkin-needed
Reporter | ||
Comment 17•8 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•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/4d32d1f87b74
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox47:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
Comment 19•8 years ago
|
||
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 20•8 years ago
|
||
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)
Flags: qe-verify+
Reporter | ||
Comment 22•8 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 23•8 years ago
|
||
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•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/d0bf1badba3c
Comment 26•8 years ago
|
||
Verified fixed FX 45 RC, 46.0a2 (2016-03-01) Win 7.
Updated•8 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•