Bug 86266 – r112643/r116697 break Webview form input fields

• Status: RESOLVED
• Resolution: FIXED
• Component: Forms

The updates realeased by Apple on 5/9/12 - 5/10/12 for Snow Leopard (OS X 10.6.8) and Lion (OS X 10.7.4) appear to include changes to Safari/Webkit that breaks some Webview form input fields.  The fields display as black on black, making them unusable.  It only appears to affect input fields of type "text" and type "password".  Here's a couple of similar (but not necessarily identical) bug reports:

https://jamfnation.jamfsoftware.com/discussion.html?id=4423&postid=21536
http://code.google.com/p/leopard-webkit/issues/detail?id=1
https://bugs.webkit.org/show_bug.cgi?id=75860

What we've found is that this can be worked around by injecting javascript into downloaded frames that sets the opacity of the affected input fields to 90+ percent. (100 percent makes them black, which is the problem).  You can also accomplish the same goal by using a user stylesheet override in WebPreferences.  Finally, enabling Core Animation for the Webview seems to fix the problem as well, but last we heard that was specifically unsupported.

[Alexey Proskuryakov]

Thank you for filing this bug!

Does this happen for you when using WebKit nightlies from <http://nightly.webkit.org>? If it does not, this is most likely not an issue with webkit.org code, and it needs to be reported to Apple directly via <http://bugrepoer.apple.com>.

[Beth Dakin]

Sounds like <rdar://problem/11400430>

[Alexey Proskuryakov]

May be duplicate of bug 82131.

[Nick Beadman]

I have seen this with the Google home page or my site with WebKit 534.57.2 (installed as part of Safari 5.1.7 on Mac OS X 10.6.8 and Mac OS X 10.7.4) and with r117340 of the WebKit source.

I have reported the issue at bugreporter.apple.com as <rdar://problem/11467914>

[Beth Dakin]

(In reply to comment #4)
> I have seen this with the Google home page or my site with WebKit 534.57.2 (installed as part of Safari 5.1.7 on Mac OS X 10.6.8 and Mac OS X 10.7.4) and with r117340 of the WebKit source.
> 
> I have reported the issue at bugreporter.apple.com as <rdar://problem/11467914>

Hi Nick, if you still have the Cocoa app that you made, can you attach it to <rdar://problem/11467914>?

[Nick Beadman]

(In reply to comment #5)
> Hi Nick, if you still have the Cocoa app that you made, can you attach it to <rdar://problem/11467914>?

@Beth: Done. Let me know if I can help in any way.

[Nick Beadman]

Internal testing has also see the problem with Safari: 5.1.6 (7534.56.5) running on Mac OS X 10.7.4

[Beth Dakin]

Thanks Nick! I have actually not been able to reproduce the bug on my machine running the same OS and the same version of Safari. I also tried on a colleague's machine and didn't see the bug. Can you think of any system or user settings that you might have that are non-standard? I'm just trying to find a common thread here of configurations that exhibit the bug.

[Nick Beadman]

(In reply to comment #8)
> Can you think of any system or user settings that you might have that are non-standard?

Not really. As you can see with the test application it is very simple. Are there any preference files that you might find useful? Certainly I have been able to reproduce it in very minimally changes Lion VMware VMs too. If necessary I can try a clean Lion install (+ updates) in VMware to see if that reproduces the issue.

[Beth Dakin]

I was able to find an instance in FileMaker Pro that I can reproduce on my machine. I don't know why I can't reproduce it more widely…that may have to do with different graphics cards or something. Anyway, I have a patch that fixes the FileMaker case I can reproduce. I will post it shortly.

[Beth Dakin]

Created attachment 142380 [details]
Patch

[Maciej Stachowiak]

Comment on attachment 142380 [details]
Patch

Test case?

[Beth Dakin]

(In reply to comment #12)
> (From update of attachment 142380 [details])
> Test case?

A DRT/WKT test case will not work since this bug does not reproduce in Safari or either of those environments either…we would see it in existing tests if it did. This also does not reproduce on my computer with a simple WebKit app. I have only been able to see the bug in FileMaker. I don't think there is a way to test thing in any of our current automated test systems.

Comment on attachment 142380 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=142380&action=review

> Source/WebCore/ChangeLog:4
> +        5/10/12 update to Snow Leopard and Lion breaks Webview form input fields

Please use either the subversion revision where this bug was introduced (it might be a revision committed to a branch) or at the very least the Safari version number where the bug first appeared (5.1.6).

> Source/WebCore/ChangeLog:15
> +        5/10/12 update. Un-styled text fields will still use NSTextFieldCell on these 

Again, please just refer to revision numbers or Safari versions.

> Source/WebCore/rendering/RenderThemeMac.mm:737
> -    bool useNewGradient = true;
> +    bool useNSTextFieldCell = true;

This variable is now only used in an interesting way in Lion and Snow Leopard, so it shouldn’t exist in the common code.

> Source/WebCore/rendering/RenderThemeMac.mm:751
> +    // We do not use NSTextFieldCell to draw styled text fields on Lion and SnowLeopard because
> +    // there are a number of bugs on those platforms that require NSTextFieldCell to be in charge
> +    // of painting its own background. We need WebCore to paint styled backgrounds, so we'll use
> +    // this WebCoreSystemInterface function instead.
> +    if (!useNSTextFieldCell) {
> +        wkDrawBezeledTextFieldCell(r, isEnabled(o) && !isReadOnlyControl(o));
> +        return false;
> +    }

This code is also specific to Lion and Snow Leopard, so it should be in the #if block for those platforms.

> Source/WebCore/rendering/RenderThemeMac.mm:753
> +    NSTextFieldCell* textField = this->textField();

Star on the wrong side of the space.

Comment on attachment 142380 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=142380&action=review

> Source/WebCore/rendering/RenderThemeMac.mm:2183
> +        // Post-Lion, WebCore can be in charnge of paintinng the background thanks to

Typo: charnge

[Beth Dakin]

Created attachment 142503 [details]
Updated patch

Thanks Dan! Here is a patch that addresses all of your comments.

[Beth Dakin]

Comment on attachment 142503 [details]
Updated patch

View in context: https://bugs.webkit.org/attachment.cgi?id=142503&action=review

> Source/WebCore/ChangeLog:21
> +        behavior for MountainLion.

Oops, forgot to include the URL for this change. I have added it to my local copy. It's http://trac.webkit.org/changeset/116697 , for reference.

[Nick Beadman]

(In reply to comment #11)

I tried my test application with svn r117453 which from everything I can tell includes the patch and it _didn't_ fix the bug.

I then started hacking on RenderThemeMac.mm and noticed:

line 2167:
NSTextFieldCell* RenderThemeMac::textField(bool useNewGradient) const

useNewGradient is always false on my Mac and therefore line 2199 is used:

        [m_textField.get() setBackgroundColor:[NSColor clearColor]];

given that changing the opacity fixes this issue (as per comment #1) is it possible that the line 2197:

        [m_textField.get() setBackgroundColor:[NSColor whiteColor]];

should be used instead? Certainly if I hack the method and set useNewGradient to true then it works for me.

I am also somewhat concerned about a line of code in RenderThemeMac::paintTextField(), line 740:

    useNewGradient = WebCore::deviceScaleFactor(o->frame()) != 1;

isn't comparing a float (WebCore::deviceScaleFactor returns a float) against 1 (int) a bad idea?

Also, after looking at Comment #10:

> that may have to do with different graphics cards or something

I tried my test application on both of the available graphics cards in my MacBookPro5,2. The problem occurs on both the integrated NVIDIA GeForce 9400M and the discrete NVIDIA GeForce 9600M GT.

Hope this helps.

(In reply to comment #18)
> (In reply to comment #11)
> 
> I tried my test application with svn r117453 which from everything I can tell includes the patch and it _didn't_ fix the bug.

Thanks for testing! This bug is still not in status FIXED, and there’s nothing here indicating the the patch was committed.

> 
> I then started hacking on RenderThemeMac.mm and noticed:
> 
> line 2167:
> NSTextFieldCell* RenderThemeMac::textField(bool useNewGradient) const
> 
> useNewGradient is always false on my Mac and therefore line 2199 is used:
> 
>         [m_textField.get() setBackgroundColor:[NSColor clearColor]];
> 
> given that changing the opacity fixes this issue (as per comment #1) is it possible that the line 2197:
> 
>         [m_textField.get() setBackgroundColor:[NSColor whiteColor]];
> 
> should be used instead? Certainly if I hack the method and set useNewGradient to true then it works for me.

Using a white background color would break text fields that specify a non-default background color.

> 
> I am also somewhat concerned about a line of code in RenderThemeMac::paintTextField(), line 740:
> 
>     useNewGradient = WebCore::deviceScaleFactor(o->frame()) != 1;
> 
> isn't comparing a float (WebCore::deviceScaleFactor returns a float) against 1 (int) a bad idea?

The compiler treats the right hand side as a floating point constant, and will compare a float to a float. The WebKit coding style guideline is to omit the unnecessary “.f” suffix in cases like this.

[Nick Beadman]

(In reply to comment #19)
> Thanks for testing! This bug is still not in status FIXED, and there’s nothing here indicating the the patch was committed.

I apologize for not having a full understanding of how WebKit development works. I assumed (incorrectly) that the patch had been applied to /trunk.

> Using a white background color would break text fields that specify a non-default background color.

I manually applied the patch (which does not have useNewGradient parameter) to my local copy of WebKit and it calls 

		[m_textField.get() setBackgroundColor:[NSColor whiteColor]];

so it does work for me.

> The compiler treats the right hand side as a floating point constant, and will compare a float to a float. The WebKit coding style guideline is to omit the unnecessary “.f” suffix in cases like this.

Thanks for the explanation. Given I had never looked at the WebKit source until yesterday I should have never questioned this.

Hopefully, the fact that I have confirmed that the patch does work (for me) helps. If you would prefer me to back off just let me know.

[Beth Dakin]

(In reply to comment #20)
> (In reply to comment #19)
> > Thanks for testing! This bug is still not in status FIXED, and there’s nothing here indicating the the patch was committed.
> 
> I apologize for not having a full understanding of how WebKit development works. I assumed (incorrectly) that the patch had been applied to /trunk.
> 

No worries! I am about to work on committing it now.

> > Using a white background color would break text fields that specify a non-default background color.
> 
> I manually applied the patch (which does not have useNewGradient parameter) to my local copy of WebKit and it calls 
> 
>         [m_textField.get() setBackgroundColor:[NSColor whiteColor]];
> 
> so it does work for me.
> 

Yay!

> > The compiler treats the right hand side as a floating point constant, and will compare a float to a float. The WebKit coding style guideline is to omit the unnecessary “.f” suffix in cases like this.
> 
> Thanks for the explanation. Given I had never looked at the WebKit source until yesterday I should have never questioned this.
> 
> Hopefully, the fact that I have confirmed that the patch does work (for me) helps. If you would prefer me to back off just let me know.

No, this is very helpful!! Thank you for confirming!!

Many people who reported this bug have nvidia graphics cards, so I do suspect that for whatever reason, those cards are more likely to exhibit the bug. I have an Intel graphics card, and I have still only ever seen the bug in FileMaker, not in your test app.

Anyway! Will commit shortly. Thanks for the review, Dan!

[Beth Dakin]

Committed patch with http://trac.webkit.org/changeset/117502

[Christiaan Hofman]

Is this really fixed? Looming at the patch, I doubt it very much. Is it clear what the real problem is? The problem is that when developing with the 10.5 SDK setting a clear background color is rendered as black, because that SDK will use NSCompositeCopy to draw instead of NSCompositeSourceOver. However, this really depends on the SDK that is used for the app that uses WebKit, it is NOT related to the SDK that is used to compile the WebKit libraries itself. Therefore checking the macros (at compile time for WebKit), as in the patch, won't work.

[Alexey Proskuryakov]

*** Bug 86895 has been marked as a duplicate of this bug. ***

[Beth Dakin]

(In reply to comment #23)
> Is this really fixed? Looming at the patch, I doubt it very much. Is it clear what the real problem is? The problem is that when developing with the 10.5 SDK setting a clear background color is rendered as black, because that SDK will use NSCompositeCopy to draw instead of NSCompositeSourceOver. However, this really depends on the SDK that is used for the app that uses WebKit, it is NOT related to the SDK that is used to compile the WebKit libraries itself. Therefore checking the macros (at compile time for WebKit), as in the patch, won't work.

Christiaan, have you actually tested the patch or are you just guessing that this is not fixed? I have verified that this patch fixes FileMaker and Nick has verified that is has fixed his applications. If you know that this bug still occurs, please give concrete steps about how to reproduce with with the latest tip of tree build of WebKit.