Closed Bug 780035 Opened 12 years ago Closed 12 years ago

font increases when typing in HTML text editor

Categories

(Core :: DOM: Editor, defect)

15 Branch
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla17
Tracking Status
firefox14 --- unaffected
firefox15 + disabled
firefox16 + verified
firefox17 + verified

People

(Reporter: keepitsimplestupid, Assigned: ayg)

References

()

Details

(Keywords: regression, reproducible)

Attachments

(3 files, 2 obsolete files)

Attached image 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
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?
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
Blocks: 752210
Status: UNCONFIRMED → NEW
Component: Untriaged → Editor
Ever confirmed: true
Keywords: regression
Product: Firefox → Core
Summary: font increases when typing → font increases when typing in HTML text editor
Looks like we've got a clear path to where this came from, cc'ing Ehsan and tracking this regression for branches.
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!
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.
Assignee: nobody → ehsan
Keywords: reproducible
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.
Assignee: ehsan → ayg
(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.
Status: NEW → ASSIGNED
Bisect says bug 590640 part 7.  Investigating.
Blocks: 590640
No longer blocks: 752210
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.
Attached patch PatchSplinter Review
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.
Attachment #651208 - Flags: review?(ehsan)
Attachment #651208 - Flags: approval-mozilla-beta?
Attachment #651208 - Flags: approval-mozilla-aurora?
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!
Attachment #651208 - Flags: review?(ehsan)
Attachment #651208 - Flags: review+
Attachment #651208 - Flags: approval-mozilla-beta?
Flags: in-testsuite+
OS: Windows 7 → All
Hardware: x86_64 → All
Attached patch Backout patch for beta (obsolete) — Splinter Review
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?)
Attachment #651676 - Flags: review?(ehsan)
Attachment #651676 - Flags: approval-mozilla-beta?
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.
Attachment #651676 - Attachment is obsolete: true
Attachment #651676 - Flags: review?(ehsan)
Attachment #651676 - Flags: approval-mozilla-beta?
Attached patch Backout patch for beta, v2 (obsolete) — Splinter Review
New try: https://tbpl.mozilla.org/?tree=Try&rev=101241b3ef5b
Attachment #651720 - Flags: review?(ehsan)
Attachment #651720 - Flags: approval-mozilla-beta?
(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.
(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!
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).
Attachment #651720 - Attachment is obsolete: true
Attachment #651720 - Flags: review?(ehsan)
Attachment #651720 - Flags: approval-mozilla-beta?
Attachment #651769 - Flags: approval-mozilla-beta?
(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.
Attachment #651769 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #651208 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/mozilla-central/rev/4b92b8cf0966
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
Keywords: verifyme
(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
Keywords: verifyme
QA Contact: paul.silaghi
Same behavior on Ubuntu 12.04 and Mac OS X 10.8.
Keywords: verifyme
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.
Status: RESOLVED → VERIFIED
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: