Closed Bug 1014246 Opened 6 years ago Closed 6 years ago

Pasting URLs no longer strips line breaks

Categories

(Core :: DOM: Editor, defect)

x86_64
All
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla32
Tracking Status
firefox29 --- wontfix
firefox30 + verified
firefox31 + verified
firefox32 + verified
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed

People

(Reporter: jruderman, Assigned: ehsan)

References

Details

(Keywords: regression)

Attachments

(1 file)

Split from bug 23485 comment 175:
>>>

I think this has recently been broken again. I've recently upgraded to 
Firefox 29.0, and now when I paste in line-broken URLs to the location bar, 
the line-breaks show up as spaces, which means they're then no longer the 
right URL

eg if you copy a URL into the address bar:
http://example.com/line-wrapped-
url/

It now no longer comes through as "http://example.com/line-wrapped-url/", as 
this bug fixed it to, but is instead "http://example.com/line-wrapped- url/" 
(which isn't the same URL so doesn't get you to the right place)

>>>

This correctly worked before. It was even fixed before, in bug 513648. I'm not sure when it broke this time.
I'm seeing this on x86 - Win 7 (Firefox v30 beta channel).
(In reply to Jesse Ruderman from comment #0)
> eg if you copy a URL into the address bar:
> http://example.com/line-wrapped-
> url/
> 
> It now no longer comes through as "http://example.com/line-wrapped-url/", as 
> this bug fixed it to, but is instead "http://example.com/line-wrapped- url/" 
> (which isn't the same URL so doesn't get you to the right place)

I get http://example.com/line-wrapped-url/ in the location bar.
I can reproduce the problem on windows7 but not on Ubuntu12.04.
So, this seems happens on Windows and MacOSX.

And, after I performed the customize mode once, the problem is gone.

Regression window(m-c)
Good:
https://hg.mozilla.org/mozilla-central/rev/626d99c084cb
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:30.0) Gecko/20100101 Firefox/30.0 ID:20140225201018
Bad:
https://hg.mozilla.org/mozilla-central/rev/0bfea5715ed4
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:30.0) Gecko/20100101 Firefox/30.0 ID:20140226030418
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=626d99c084cb&tochange=0bfea5715ed4

Regression window(fx)
Good:
https://hg.mozilla.org/integration/fx-team/rev/11b0efa84735
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:30.0) Gecko/20100101 Firefox/30.0 ID:20140225221421
Bad:
https://hg.mozilla.org/integration/fx-team/rev/0bfea5715ed4
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:30.0) Gecko/20100101 Firefox/30.0 ID:20140225230820
Pushlog:
http://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=11b0efa84735&tochange=0bfea5715ed4
>I can reproduce the problem on windows7 but not on Ubuntu12.04.
>So, this seems happens on Windows and MacOSX.

Error,
This happens on ubuntu too.
OS: Mac OS X → All
(In reply to Alice0775 White from comment #3)
> I can reproduce the problem on windows7 but not on Ubuntu12.04.
> So, this seems happens on Windows and MacOSX.
> 
> And, after I performed the customize mode once, the problem is gone.
> 
> Regression window(m-c)
> Good:
> https://hg.mozilla.org/mozilla-central/rev/626d99c084cb
> Mozilla/5.0 (Windows NT 6.1; WOW64; rv:30.0) Gecko/20100101 Firefox/30.0
> ID:20140225201018
> Bad:
> https://hg.mozilla.org/mozilla-central/rev/0bfea5715ed4
> Mozilla/5.0 (Windows NT 6.1; WOW64; rv:30.0) Gecko/20100101 Firefox/30.0
> ID:20140226030418
> Pushlog:
> http://hg.mozilla.org/mozilla-central/
> pushloghtml?fromchange=626d99c084cb&tochange=0bfea5715ed4
> 
> Regression window(fx)
> Good:
> https://hg.mozilla.org/integration/fx-team/rev/11b0efa84735
> Mozilla/5.0 (Windows NT 6.1; WOW64; rv:30.0) Gecko/20100101 Firefox/30.0
> ID:20140225221421
> Bad:
> https://hg.mozilla.org/integration/fx-team/rev/0bfea5715ed4
> Mozilla/5.0 (Windows NT 6.1; WOW64; rv:30.0) Gecko/20100101 Firefox/30.0
> ID:20140225230820
> Pushlog:
> http://hg.mozilla.org/integration/fx-team/
> pushloghtml?fromchange=11b0efa84735&tochange=0bfea5715ed4

This regression range doesn't make sense if 29 is unaffected. Even if it did, this is unrelated to the australis merge (which was in november 2013).
No longer blocks: australis-merge
(In reply to Jesse Ruderman from comment #0)
> Split from bug 23485 comment 175:
> >>>
> 
> I think this has recently been broken again. I've recently upgraded to 
> Firefox 29.0, and now when I paste in line-broken URLs to the location bar, 
> the line-breaks show up as spaces, which means they're then no longer the 
> right URL
> 
> eg if you copy a URL into the address bar:
> http://example.com/line-wrapped-
> url/
> 
> It now no longer comes through as "http://example.com/line-wrapped-url/", as 
> this bug fixed it to, but is instead "http://example.com/line-wrapped- url/" 
> (which isn't the same URL so doesn't get you to the right place)
> 
> >>>
> 
> This correctly worked before. It was even fixed before, in bug 513648. I'm
> not sure when it broke this time.

Bug 513648 was about pasting in the content of the window with the middlemouseclick (which navigates by default on Linux) and not about the URL bar, which bug 513648 comment #0 explicitly says worked at the time.
Flags: firefox-backlog+
(In reply to :Gijs Kruitbosch from comment #5)
> (In reply to Alice0775 White from comment #3)
> > I can reproduce the problem on windows7 but not on Ubuntu12.04.
> > So, this seems happens on Windows and MacOSX.
> > 
> > And, after I performed the customize mode once, the problem is gone.
> > 
> > Regression window(m-c)
> > Good:
> > https://hg.mozilla.org/mozilla-central/rev/626d99c084cb
> > Mozilla/5.0 (Windows NT 6.1; WOW64; rv:30.0) Gecko/20100101 Firefox/30.0
> > ID:20140225201018
> > Bad:
> > https://hg.mozilla.org/mozilla-central/rev/0bfea5715ed4
> > Mozilla/5.0 (Windows NT 6.1; WOW64; rv:30.0) Gecko/20100101 Firefox/30.0
> > ID:20140226030418
> > Pushlog:
> > http://hg.mozilla.org/mozilla-central/
> > pushloghtml?fromchange=626d99c084cb&tochange=0bfea5715ed4
> > 
> > Regression window(fx)
> > Good:
> > https://hg.mozilla.org/integration/fx-team/rev/11b0efa84735
> > Mozilla/5.0 (Windows NT 6.1; WOW64; rv:30.0) Gecko/20100101 Firefox/30.0
> > ID:20140225221421
> > Bad:
> > https://hg.mozilla.org/integration/fx-team/rev/0bfea5715ed4
> > Mozilla/5.0 (Windows NT 6.1; WOW64; rv:30.0) Gecko/20100101 Firefox/30.0
> > ID:20140225230820
> > Pushlog:
> > http://hg.mozilla.org/integration/fx-team/
> > pushloghtml?fromchange=11b0efa84735&tochange=0bfea5715ed4
> 
> This regression range doesn't make sense if 29 is unaffected. Even if it
> did, this is unrelated to the australis merge (which was in november 2013).

I mean this is Australis related bug. So I add 939862.

Any way, The regression range is correct.
Something changed in that was 29beta/aurora.
Firefox29 is also affected.
In local build
Last Good: 434a7a69d46f
First Bad: aa09dfcfce31

Regressed by:
aa09dfcfce31	Matthew Noorenberghe — Bug 963576 - Part 1 - Provide attributes/classes to style customization targets. r=Gijs
Blocks: 963576
(In reply to Alice0775 White from comment #9)
> In local build
> Last Good: 434a7a69d46f
> First Bad: aa09dfcfce31
> 
> Regressed by:
> aa09dfcfce31	Matthew Noorenberghe — Bug 963576 - Part 1 - Provide
> attributes/classes to style customization targets. r=Gijs

I have literally 0 ideas on how this could cause this feature to break. Ehsan, AFAICT the editor newline=somethingorother thing is breaking here. Do you have cycles to investigate why that is?
Flags: needinfo?(ehsan)
(In reply to :Gijs Kruitbosch from comment #10)
> (In reply to Alice0775 White from comment #9)
> > In local build
> > Last Good: 434a7a69d46f
> > First Bad: aa09dfcfce31
> > 
> > Regressed by:
> > aa09dfcfce31	Matthew Noorenberghe — Bug 963576 - Part 1 - Provide
> > attributes/classes to style customization targets. r=Gijs
> 
> I have literally 0 ideas on how this could cause this feature to break.
> Ehsan, AFAICT the editor newline=somethingorother thing is breaking here. Do
> you have cycles to investigate why that is?

Why do you think this is editor related?  The code for the newline handling in the editor has not changed at least in the past couple of years.  Also, comment 9 and comment 3 clearly pinpoint this to the customize mode.
Flags: needinfo?(ehsan)
WORKAROUND #1
Toggle customize mode once.

WORKAROUND #2
Evaluate the following code in Browser Console.

gURLBar.parentNode.replaceChild(gURLBar, gURLBar);
(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #11)
> (In reply to :Gijs Kruitbosch from comment #10)
> > (In reply to Alice0775 White from comment #9)
> > > In local build
> > > Last Good: 434a7a69d46f
> > > First Bad: aa09dfcfce31
> > > 
> > > Regressed by:
> > > aa09dfcfce31	Matthew Noorenberghe — Bug 963576 - Part 1 - Provide
> > > attributes/classes to style customization targets. r=Gijs
> > 
> > I have literally 0 ideas on how this could cause this feature to break.
> > Ehsan, AFAICT the editor newline=somethingorother thing is breaking here. Do
> > you have cycles to investigate why that is?
> 
> Why do you think this is editor related?  The code for the newline handling
> in the editor has not changed at least in the past couple of years.  Also,
> comment 9 and comment 3 clearly pinpoint this to the customize mode.

Because the problem occurs when the user /doesn't/ enter customize mode? The only thing even vaguely related to customize mode is that doing any kind of DOM manipulation that reinserts the node (like entering customize mode) makes things work, but considering that even a node.parentNode.replaceChild(node, node) does that, that hardly has anything to do with customize mode...

I think it's editor related because nothing is touching that attribute (newline) and yet it doesn't actually work. The editor is what's stripping the newlines here, and that's what isn't happening. Considering that re-inserting the node makes everything work again, it seems logical that the issue here has to do with /something/ breaking editor initialization/configuration in a way that means this attribute no longer does what it's meant to do. Because the implementation of the newline attribute is all in C++, I have no idea where to even start debugging, and because you effectively own editor these days, I'm asking you if you can help us out here.
Flags: needinfo?(ehsan)
Gijs, I'm _trying_ to help here.

Comment 12 actually suggests that this might be a reframing problem.  I seem to remember that we used to set the newline handling behavior somewhere in urlbarBindings.xml but I can't find it any more.  Looking for the callers of http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/textbox.xml#132, I can only see one test that is calling it!  Who's responsible to set the newline handling mode on the URL bar these days?
Flags: needinfo?(ehsan)
I swear I debugged this issue once!  What's happening is that mNewlineHandling is set by the XBL constructor for the <textbox> element, and then we reinit the editor and reset the value from the pref (which is "replace with space").  Presumably the reason why this is triggered by Australis is that it changes when/if we reinit the editor.
Component: Location Bar → Editor
Product: Firefox → Core
Attached patch Patch (v1)Splinter Review
Assignee: nobody → ehsan
Status: NEW → ASSIGNED
Attachment #8427299 - Flags: review?(roc)
(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #14)
> Gijs, I'm _trying_ to help here.

And succeeding, I think! :-)

I apologize if I came across harsh earlier. I should have included more information in the initial needinfo, and more understanding later when it wasn't immediately obvious why I needinfo'd you. My fault. Thanks a lot for diving in. :-)
No worries.  :-)
Flags: firefox-backlog+ → firefox-backlog-
https://hg.mozilla.org/mozilla-central/rev/31909569dc5d
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
This looks like a simple enough fix - worth uplifting to Beta?  If so, let's get this into our Thursday beta and avoid shipping this annoying regression again.
Flags: needinfo?(ehsan)
Comment on attachment 8427299 [details] [diff] [review]
Patch (v1)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): This bug has been there since forever, but apparently Australis has caused it to trigger more often.
User impact if declined: Comment 0
Testing completed (on m-c, etc.): locally
Risk to taking this patch (and alternatives if risky): not risky
String or IDL/UUID changes made by this patch: none
Attachment #8427299 - Flags: approval-mozilla-beta?
Attachment #8427299 - Flags: approval-mozilla-aurora?
Flags: needinfo?(ehsan)
Attachment #8427299 - Flags: approval-mozilla-beta?
Attachment #8427299 - Flags: approval-mozilla-beta+
Attachment #8427299 - Flags: approval-mozilla-aurora?
Attachment #8427299 - Flags: approval-mozilla-aurora+
(In reply to Jesse Ruderman from comment #0)
> eg if you copy a URL into the address bar:
> http://example.com/line-wrapped-
> url/
> 
> It now no longer comes through as "http://example.com/line-wrapped-url/",
Now, if adding some extra spaces at the end of the first line and at the beginning of the second line, the result is still "http://example.com/line-wrapped-url/". I think it should be "http://example.com/line-wrapped-     url/"
Thoughts?
Flags: needinfo?(ehsan)
(In reply to Paul Silaghi, QA [:pauly] from comment #26)
> (In reply to Jesse Ruderman from comment #0)
> > eg if you copy a URL into the address bar:
> > http://example.com/line-wrapped-
> > url/
> > 
> > It now no longer comes through as "http://example.com/line-wrapped-url/",
> Now, if adding some extra spaces at the end of the first line and at the
> beginning of the second line, the result is still
> "http://example.com/line-wrapped-url/". I think it should be
> "http://example.com/line-wrapped-     url/"
> Thoughts?

That is expected.  The buggy behavior was replacing a newline character with a space character.  Spaces in the original string should be left untouhced, which seems to be what you're seeing.
Flags: needinfo?(ehsan)
My understanding is that spaces that are contiguous with line breaks are stripped, and that that is the intended behavior.
(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #27)
> (In reply to Paul Silaghi, QA [:pauly] from comment #26)
> > (In reply to Jesse Ruderman from comment #0)
> > > eg if you copy a URL into the address bar:
> > > http://example.com/line-wrapped-
> > > url/
> > > 
> > > It now no longer comes through as "http://example.com/line-wrapped-url/",
> > Now, if adding some extra spaces at the end of the first line and at the
> > beginning of the second line, the result is still
> > "http://example.com/line-wrapped-url/". I think it should be
> > "http://example.com/line-wrapped-     url/"
> > Thoughts?
> 
> That is expected.  The buggy behavior was replacing a newline character with
> a space character.  Spaces in the original string should be left untouhced,
> which seems to be what you're seeing.

(In reply to Jesse Ruderman from comment #28)
> My understanding is that spaces that are contiguous with line breaks are
> stripped, and that that is the intended behavior.

The two comments above are opposite. What is the intended behavior?

After this patch, I see the new behavior is what Jesse said, ie., spaces in first line plus the line-break and spaces at the beginning of second line are both stripped.

Exemplified:

According to Ehsan and Paul,
> https://bugzilla.mozilla.org/show_•••
> •••bug.cgi?id=1014246
should turn to
> https://bugzilla.mozilla.org/show_••••••bug.cgi?id=1014246
(where • = space)

But after the path, I see:
> https://bugzilla.mozilla.org/show_•••
> •••bug.cgi?id=1014246
converted to
> https://bugzilla.mozilla.org/show_bug.cgi?id=1014246
(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #27)
> Spaces in the original string should be left untouhced,
> which seems to be what you're seeing.
This is actually what I was saying it's not working
(In reply to Jesse Ruderman from comment #28)
> My understanding is that spaces that are contiguous with line breaks are
> stripped, and that that is the intended behavior.
Looks inconsistent to me, other spaces (that aren't contiguous with line breaks) are kept.
Duplicate of this bug: 1018218
per comments 30, 31
Flags: needinfo?(ehsan)
Flags: needinfo?(jruderman)
Like Jesse said, the default behavior is "stripsurroundingwhitespace", so the behavior that you see is expected.  FWIW I did verify this bug when I fixed it, we don't really need to spend more time on it.  :-)
Status: RESOLVED → VERIFIED
Flags: needinfo?(jruderman)
Flags: needinfo?(ehsan)
Ok then, verified also on FF 30 RC and 31.0a2 (2014-06-03) win 7 x64
You need to log in before you can comment on or make changes to this bug.