780035 - font increases when typing in HTML text editor

Status: VERIFIED FIXED

(Core :: DOM: Editor, defect)

Description

[keepitsimplestupid]

Attachment: CR4 Font issues.PNG

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:15.0) Gecko/20100101 Firefox/15.0
Build ID: 20120724191344

Steps to reproduce:

Updated to FF 15.


Actual results:

when typing into a website, the font increases.  Someone else has noticed the same issue.

Url of both others reporting the same bug and where the bug occurred.


Expected results:

The font should not jump point size

Comment 1

[keepitsimplestupid]

All you have to do is to respond to a question on this forum and usually during the first line the font size does big magnify.  The forums tools will squish it back before posting.

Lately, Firefox is just getting way to unstable to even use.

I believe there are other subtle bugs when typing into text boxes.  Can I go back to a previous version to see when another intermittent bug cropped up?

Comment 2

[Loic]

There is a regression in FF15+ indeed, thanks for reporting.

STR are simple, just create a fake account to http://cr4.globalspec.com/user/register and simulate to post a new message to use the HTML text editor.

Mozregression range:

m-c
good=2012-05-18
bad=2012-05-19
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=e794cef56df6&tochange=642d1a36702f

Suspected bug:
Bug 752210 - Use nsIContent in nsHTMLEditor::RelativeFontChange

Comment 3

[Lukas Blakk [:lsblakk] use ?needinfo]

Looks like we've got a clear path to where this came from, cc'ing Ehsan and tracking this regression for branches.

Comment 4

[keepitsimplestupid]

OK, guys I have finally managed to be able to duplicate a likely related bug

On CR4 and even Bugzilla and vbulletin forums will do it (probably any text box), do the following:

The SHIFT key used must be the LEFT SHIFT key.  The RIGHT SHIFT key seems to be immune.

With a blank Text BOX visible.

Type "EXTREMELY QUICKLY" a line of gibberish no return such as:
ksdksadkldjkladklasjdklsdklaskldskljd
and IMMEDIATELY press a SHIFT+R and release the SHIFT key 

Wait about 1 second or so and type a SHIFT+R at the normal rate, do the normal SHIFT+R again if necessary. 

Do it a few times because timing is everything. The SHIFT+R will refresh the page.

You may have to refresh the page.

This has been driving me nuts.  Other keys are interpreted this way too such as D and B which will be interpreted as a control character.  Although I did not try this test on them.

This has been the FIRST time that I managed to create a way of duplicating the bug with a reasonable success rate, about 50% of the time.

Thank you!

Comment 5

[Lukas Blakk [:lsblakk] use ?needinfo]

Thanks for the detailed comment, assigning to Ehsan for investigation - please re-assign to someone else if you're not the right person for this, but at this stage in the release cycle it's important to make sure all tracked bugs have assignees working on them.

Comment 6

[(no longer active)]

Aryeh, can you please take a look at this?  I would start by making sure that you can reproduce, and then bisecting the regression range.

Comment 7

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

(In reply to Loic from comment #2)
> There is a regression in FF15+ indeed, thanks for reporting.
> 
> STR are simple, just create a fake account to
> http://cr4.globalspec.com/user/register and simulate to post a new message
> to use the HTML text editor.

Confirmed.  Looking.  Hitting Enter seems to create an extra <font size="13.3333px"> for some strange reason.  This is consistently reproducible.  It looks like they use their own custom HTML editor.

> Mozregression range:
> 
> m-c
> good=2012-05-18
> bad=2012-05-19
> http://hg.mozilla.org/mozilla-central/
> pushloghtml?fromchange=e794cef56df6&tochange=642d1a36702f
> 
> Suspected bug:
> Bug 752210 - Use nsIContent in nsHTMLEditor::RelativeFontChange

There are a zillion editor-related bugs in that range, so I don't know if that's actually the culprit.  Bug 590640 also is a possibility, but there are a boatload of cleanup patches too.  I'll do a more detailed bisect -- thanks for the nightly range.

(In reply to keepitsimplestupid from comment #4)
> OK, guys I have finally managed to be able to duplicate a likely related bug
> 
> On CR4 and even Bugzilla and vbulletin forums will do it (probably any text
> box), do the following:
> 
> The SHIFT key used must be the LEFT SHIFT key.  The RIGHT SHIFT key seems to
> be immune.
> 
> With a blank Text BOX visible.
> 
> Type "EXTREMELY QUICKLY" a line of gibberish no return such as:
> ksdksadkldjkladklasjdklsdklaskldskljd
> and IMMEDIATELY press a SHIFT+R and release the SHIFT key 
> 
> Wait about 1 second or so and type a SHIFT+R at the normal rate, do the
> normal SHIFT+R again if necessary. 
> 
> Do it a few times because timing is everything. The SHIFT+R will refresh the
> page.
> 
> You may have to refresh the page.
> 
> This has been driving me nuts.  Other keys are interpreted this way too such
> as D and B which will be interpreted as a control character.  Although I did
> not try this test on them.
> 
> This has been the FIRST time that I managed to create a way of duplicating
> the bug with a reasonable success rate, about 50% of the time.

This doesn't seem to work for me at all.  Shift+R doesn't refresh the page.  But I can reproduce reliably enough at globalspec.com, and that should be enough.

Comment 8

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Bisect says bug 590640 part 7.  Investigating.

Comment 9

[keepitsimplestupid]

The Left Shift+R bug is elusive for me too.  It also affects D and B and I think some others, but does not affect the RIGHT-SHIFT key.  Once it happens, the quick typing thing seems to reproduce it.  It's on a laptop and the Keyboard has been replaced.  A Staples tech has said that he has seen the bug. I think it's just Firefox. Here are some other references.

http://raywoodcockslatest.blogspot.com/2012/04/windows-7-shift-and-control-not-caps.html
http://www.howtogeek.com/forum/topic/my-left-shift-key-has-problems
http://answers.microsoft.com/en-us/windows/forum/windows_7-windows_programs/stuck-shift-keys-windows-7-64-bit/8b1dc731-63d5-4da9-bf20-4a34300fba95
http://answers.microsoft.com/en-us/windows/forum/windows_7-hardware/left-shift-key-not-working/f71561f0-7fd6-4e71-81ff-5ca6fd8aa58f?msgId=70272baa-857c-484c-9903-de4fcf322df1
http://mgmcc.forumotion.net/t405-shift-key-problems
http://www.howtogeek.com/forum/topic/my-left-shift-key-has-problems

Comment 10

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

So something in CacheInlineStyles thinks that there's a font size set, which it caches -- a value of 13.3333px.  But then something else later tries to reapply the style . . . in the form of <font size=13.3333px>.  But size="" takes a numeric argument, not a length argument, so the unit is ignored and this parses as <font size=13.3333>, which then gets capped so it's the same as <font size=7>.  Which is, um, not intended.  There's some underlying bug here that bug 590640 must have triggered by exposing this style caching in more places.

Comment 11

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Patch

So the issue here is basically that editor/ is completely broken and I hate it.  Specifically, it deals with styles as tag/attribute pairs, which doesn't map correctly to the capabilities we actually support in some cases.  And CSS support is bolted on after the fact, and not all tag/attribute pairs really support CSS.  This code really really needs to be refactored.  I worked around the problem here by just forcing us into non-CSS mode for <font size> in this specific case, which is really an ugly band-aid but seems to work.

In principle this should be more correct than what we had before -- given the way our <font size> support works, calling IsCSSEquivalentToHTMLInlineStyleSet for it is basically always going to be wrong.  (IsCSSEquivalentToHTMLInlineStyleSet should really be the same function as IsTextPropertySetByContent, so callers don't have to decide how to use them, but it's not, because CSS support is a complete hack . . .)

Try: https://tbpl.mozilla.org/?tree=Try&rev=5c8edca354ef


[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 590640, part 7

User impact if declined: <font size=7> will sometimes be incorrectly added to rich-text editors.  I believe this will happen if a) an explicit CSS font-size is specified somewhere other than the default, and b) styleWithCSS mode is *true* (which is not the default and very few pages override the default).  The bug is not likely to occur often, but when it does, it might render the rich-text editor extremely annoying to use.

Testing completed (on m-c, etc.): None.  This is not a partial or complete backout; it introduces new behavior.

Risk to taking this patch (and alternatives if risky): Unclear.  I wouldn't say it's high-risk, but I can't say there's no risk either.  I'm pretty sure it's better to back out the regressing patch instead, but I'd like to hear what Ehsan has to say.

String or UUID changes made by this patch: None.

Comment 12

[(no longer active)]

Comment on attachment 651208 [details] [diff] [review]
Patch

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

Hmm, yeah this is not great but I don't see any other easy solution here...

Unfortunately I think this is very risky for beta specially this late in the cycle.  I'm fine with this patch landing on Aurora to get more testing and I think it will probably be fine, but there's a web compat risk that we cannot ignore.  Which means that bug 590640 needs to get backed out from Beta.  :(

Can you please attach a patch for that?  Thanks!

Comment 13

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4b92b8cf0966

Comment 14

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Backout patch for beta

hg reported a conflict in test_runtest.html.json, i.e., the list of expected fails.  It was trivial to resolve, but I don't know if it was resolved correctly.  Try: https://tbpl.mozilla.org/?tree=Try&rev=0487491f8988

(I didn't verify that this actually fixes the reported regression, but assuming it works at all, it should.  Should I add tests to backouts like this to verify that they fix the regression, or keep them just backouts?)

Comment 15

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Comment on attachment 651676 [details] [diff] [review]
Backout patch for beta

The JSON file here has a syntax error.  Will submit a new patch shortly.

Comment 16

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Backout patch for beta, v2

New try: https://tbpl.mozilla.org/?tree=Try&rev=101241b3ef5b

Comment 17

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

(In reply to :Aryeh Gregor from comment #16)
> Created attachment 651720 [details] [diff] [review]
> Backout patch for beta, v2
> 
> New try: https://tbpl.mozilla.org/?tree=Try&rev=101241b3ef5b

This has a bunch of unexpected passes for test_runtest.html.  I'll write a new patch to fix those in a little bit.

Comment 18

[(no longer active)]

(In reply to :Aryeh Gregor from comment #14)
> (I didn't verify that this actually fixes the reported regression, but
> assuming it works at all, it should.  Should I add tests to backouts like
> this to verify that they fix the regression, or keep them just backouts?)

Yes, please verify that the backout fixes the regression (you can use the try builds if you want to avoid a local build).  Also, backout patches don't really need reviews.  Please just request approval for beta when it passes try.  Thanks!

Comment 19

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Backout patch for beta, v3

This compiles and passes test_runtest.html on localhost, so I expect the try run to be a success this time: https://tbpl.mozilla.org/?tree=Try&rev=aa3a2d98b958

I also verified on localhost that it seems to not have the bug, although I didn't add the test that was checked into m-c/aurora (tell me if I should).

Comment 20

[(no longer active)]

(In reply to comment #19)
> I also verified on localhost that it seems to not have the bug, although I
> didn't add the test that was checked into m-c/aurora (tell me if I should).

No, manual verification is all that is needed here.

Comment 21

[(no longer active)]

Fixed on Aurora: http://hg.mozilla.org/releases/mozilla-aurora/rev/f9db91caea8d
Disabled on Beta: http://hg.mozilla.org/releases/mozilla-beta/rev/2b306c236558

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4b92b8cf0966

Comment 23

[Paul Silaghi, QA [:pauly]]

(In reply to keepitsimplestupid from comment #0)
> Created attachment 648553 [details]
> CR4 Font issues.PNG
> 
> User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:15.0) Gecko/20100101
> Firefox/15.0
> Build ID: 20120724191344
> 
> Steps to reproduce:
> 
> Updated to FF 15.
> 
> 
> Actual results:
> 
> when typing into a website, the font increases.  Someone else has noticed
> the same issue.
> 
> Url of both others reporting the same bug and where the bug occurred.
> 
> 
> Expected results:
> 
> The font should not jump point size

Tested on http://cr4.globalspec.com/thread/79696?frmtrk=cr4sd#newcomments.
Verified fixed on Mozilla/5.0 (Windows NT 6.1; WOW64; rv:15.0) Gecko/20100101 Firefox/15.0.1 due to backout of bug 590640 from FF 15.
Verified fixed on Mozilla/5.0 (Windows NT 6.1; WOW64; rv:16.0) Gecko/20100101 Firefox/16.0b2

Comment 24

[Paul Silaghi, QA [:pauly]]

Same behavior on Ubuntu 12.04 and Mac OS X 10.8.

Comment 25

[Paul Silaghi, QA [:pauly]]

Tested on http://cr4.globalspec.com/, http://www.loganclub.ro/forum/
Verified fixed on FF 17b1 on Win 7 x64, Ubuntu 12.04 and Mac OS X 10.6.8.