1034077 - Location bar shouldn't strip whitespace when pasting

Status: RESOLVED FIXED

(Firefox :: Address Bar, defect, P1)

Description

[Andrei Eftimie]

Module:    testPasteLocationBar
Test:      /testAwesomeBar/testPasteLocationBar.js    
Failure:   Location bar should contain pasted clipboard content - expected ipsum
Branches:  default
Platforms: Win
Report:    http://mozmill-daily.blargon7.com/#/functional/report/8e52cc909fb28910195c4e255e31f105

So far only reported failures are on windows.

Comment 1

[Andrei Eftimie]

Attachment: skip.patch

Only Windows is affected, no need to disable the test on other platforms.

Comment 2

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8450201 [details] [diff] [review]
skip.patch

Review of attachment 8450201 [details] [diff] [review]:
-----------------------------------------------------------------

Disabled:
http://hg.mozilla.org/qa/mozmill-tests/rev/af856bfc3f97 (default)

Comment 3

[Andrei Eftimie]

I can reproduce it manually.
We get an extra whitespace character selected when double clicking on a word.

Steps:
1) open a webpage (eg. mozilla.org)
2) double click a word (eg: "are")

Expected selection: "are"
Actual selection: "are "

Comment 4

[Andrei Eftimie]

Pushlog is pretty big: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=7075808c3306&tochange=38ecfc3922b8

Nothing stands out related to text selection. Will probably have to run some tinderbox builds.

Comment 5

[Daniel Gherasim]

Here is the pushlog:
http://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=5bc01fa51b24&tochange=740ef090bec0

What actually changed is that text is stripped from spaces at the beginning/end when we paste it in the url bar.

I'm not sure about the default behavior in firefox on windows when we select a word (it also selects the space after the word) that exists for a while.

Comment 6

[:Gijs (he/him)]

Hrm. I don't think it was intentional that the patch from bug 1018154 strips leading and trailing whitespace. On the other hand... I can't well imagine a case where it's problematic. Dão, should we fix this? I lean on the side of 'yes', but I'd like your opinion before I try and write a patch.

Comment 7

[Dão Gottwald [:dao]]

This sounds like something we should fix, since users may want to type after pasting and expect the space to be there.

Comment 8

[:Gijs (he/him)]

Attachment: url bar shouldn't strip whitespace on all pastes,

Making the strip function not alter the result because (a) that's what it says on the tin, and (b) both consumers currently expect this, and that seems more sane than the alternative, even if I find the current structure in the function slightly kludgy...

Comment 9

[Marco Mucci [:MarcoM]]

Added to Iteration 33.2

Comment 10

[Dão Gottwald [:dao]]

Comment on attachment 8450977 [details] [diff] [review]
url bar shouldn't strip whitespace on all pastes,

Using pastedURI as a proxy for "this is at least the second loop run" makes this too weirdly structured for my taste at this point. Can you initialize pastedURI outside of the loop and make it a while-loop?

Comment 11

[:Gijs (he/him)]

Attachment: url bar shouldn't strip whitespace on all pastes,

Like this?

Comment 12

[Dão Gottwald [:dao]]

Attachment: patch

(In reply to :Gijs Kruitbosch from comment #11)
> Created attachment 8450990 [details] [diff] [review]
> url bar shouldn't strip whitespace on all pastes,
> 
> Like this?

Kind of. I think this can be simplified significantly, though.

Comment 13

[:Gijs (he/him)]

Comment on attachment 8450998 [details] [diff] [review]
patch

Review of attachment 8450998 [details] [diff] [review]:
-----------------------------------------------------------------

The reason that I didn't go this far is that it behaves differently to my patch if you paste something that parses as a URI (when trimmed), e.g. "http://x ". I'm not sure we should let that happen, especially what with the "manual paste" behaviour that we use in urlbarBindings.

Comment 14

[:Gijs (he/him)]

Comment on attachment 8450998 [details] [diff] [review]
patch

Review of attachment 8450998 [details] [diff] [review]:
-----------------------------------------------------------------

Hrm, no it wouldn't. I missed that you're only trimming the input to makeURL.

Comment 15

[Dão Gottwald [:dao]]

https://hg.mozilla.org/integration/fx-team/rev/d031c88447ac

Comment 16

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Gijs, thanks for the quick fix. For the next time could we please create a new bug for fixing the core issue, instead of moving it away from our own component?  It kills our tracking. Thanks.

Comment 17

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/d031c88447ac

Comment 18

[Andrei Eftimie]

Tested on a tinderbox mc build, fix works fine.
Backed out skip: https://hg.mozilla.org/qa/mozmill-tests/rev/ef1acb57fd2b (default)

Comment 19

[James Ross]

(In reply to :Gijs Kruitbosch from comment #6)
> Hrm. I don't think it was intentional that the patch from bug 1018154 strips
> leading and trailing whitespace.

I thought it was, for the case listed in bug 1018154 comment 1 and Chrome/IE parity.

Comment 20

[:Gijs (he/him)]

(In reply to James Ross from comment #19)
> (In reply to :Gijs Kruitbosch from comment #6)
> > Hrm. I don't think it was intentional that the patch from bug 1018154 strips
> > leading and trailing whitespace.
> 
> I thought it was, for the case listed in bug 1018154 comment 1 and Chrome/IE
> parity.

We strip it for "dangerous" URL cases, but not if you paste in "foopy ".

Comment 21

[Marco Mucci [:MarcoM]]

Hi Florin, can a QA contact be assigned for verification of this bug.  Dao provided the following on how to verify, "One just needs to copy a string with a trailing space, e.g. "foo ". When pasted into the location bar, the trailing space should be preserved."

Comment 22

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Our Mozmill test, which detected this regression by accident, is passing again. So I don't think it needs manual verification. But it would be good to have a specific test, which we can run on buildbot.

Comment 23

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Marking as [qa-] based on Henrik's comment 22.