Closed
Bug 1014246
Opened 11 years ago
Closed 11 years ago
Pasting URLs no longer strips line breaks
Categories
(Core :: DOM: Editor, defect)
Tracking
()
People
(Reporter: jruderman, Assigned: ehsan.akhgari)
References
Details
(Keywords: regression)
Attachments
(1 file)
967 bytes,
patch
|
roc
:
review+
lsblakk
:
approval-mozilla-aurora+
lsblakk
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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.
Comment 1•11 years ago
|
||
I'm seeing this on x86 - Win 7 (Firefox v30 beta channel).
Assignee | ||
Updated•11 years ago
|
Keywords: regressionwindow-wanted
Comment 2•11 years ago
|
||
(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.
Comment 3•11 years ago
|
||
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
status-firefox29:
--- → unaffected
status-firefox30:
--- → affected
status-firefox31:
--- → affected
status-firefox32:
--- → affected
Comment 4•11 years ago
|
||
>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
Updated•11 years ago
|
Keywords: regressionwindow-wanted
Updated•11 years ago
|
Blocks: australis-merge
Comment 5•11 years ago
|
||
(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
Comment 6•11 years ago
|
||
(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+
Comment 7•11 years ago
|
||
(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.
Comment 8•11 years ago
|
||
Firefox29 is also affected.
Updated•11 years ago
|
Comment 9•11 years ago
|
||
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
Comment 10•11 years ago
|
||
(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)
Assignee | ||
Comment 11•11 years ago
|
||
(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)
Comment 12•11 years ago
|
||
WORKAROUND #1
Toggle customize mode once.
WORKAROUND #2
Evaluate the following code in Browser Console.
gURLBar.parentNode.replaceChild(gURLBar, gURLBar);
Comment 13•11 years ago
|
||
(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)
Assignee | ||
Comment 14•11 years ago
|
||
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)
Assignee | ||
Comment 15•11 years ago
|
||
Ah, nevermind, this is how: <http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.xul#676>
Assignee | ||
Comment 16•11 years ago
|
||
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
Assignee | ||
Comment 17•11 years ago
|
||
Attachment #8427299 -
Flags: review?(roc) → review+
Comment 18•11 years ago
|
||
(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. :-)
Assignee | ||
Comment 19•11 years ago
|
||
No worries. :-)
Assignee | ||
Comment 20•11 years ago
|
||
Updated•11 years ago
|
Flags: firefox-backlog+ → firefox-backlog-
Comment 21•11 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Comment 22•11 years ago
|
||
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)
Assignee | ||
Comment 23•11 years ago
|
||
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)
Updated•11 years ago
|
Attachment #8427299 -
Flags: approval-mozilla-beta?
Attachment #8427299 -
Flags: approval-mozilla-beta+
Attachment #8427299 -
Flags: approval-mozilla-aurora?
Attachment #8427299 -
Flags: approval-mozilla-aurora+
Updated•11 years ago
|
Comment 24•11 years ago
|
||
Comment 25•11 years ago
|
||
status-b2g-v1.4:
--- → fixed
status-b2g-v2.0:
--- → fixed
Comment 26•11 years ago
|
||
(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)
Assignee | ||
Comment 27•11 years ago
|
||
(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)
Reporter | ||
Comment 28•11 years ago
|
||
My understanding is that spaces that are contiguous with line breaks are stripped, and that that is the intended behavior.
Comment 29•11 years ago
|
||
(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
Comment 30•11 years ago
|
||
(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
Comment 31•11 years ago
|
||
(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.
Comment 33•11 years ago
|
||
per comments 30, 31
Updated•11 years ago
|
Flags: needinfo?(ehsan)
Updated•11 years ago
|
Flags: needinfo?(jruderman)
Assignee | ||
Comment 34•11 years ago
|
||
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)
Comment 35•11 years ago
|
||
Ok then, verified also on FF 30 RC and 31.0a2 (2014-06-03) win 7 x64
Keywords: verifyme
You need to log in
before you can comment on or make changes to this bug.
Description
•