Bug 50398 – Android would like to disable rounding hacks for TextRuns in text controls

• Status: RESOLVED
• Resolution: WONTFIX
• Component: Platform

Created attachment 75390 [details]
git diff of the changes I'd like to make.

Android would like to disable rounding hacks for TextRuns in text controls

Created attachment 75391 [details]
A patch using svn diff

Created attachment 75401 [details]
Disable rounding hacks for TextControls

Created attachment 75710 [details]
ChangeLog

[WebKit Review Bot]

Attachment 75710 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--diff-files']" exit_code: 1
Total errors found: 0 in 0 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Steve Block]

Comment on attachment 75401 [details]
Disable rounding hacks for TextControls

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

I saw that you uploaded a ChangeLog entry separately. You'll have to merge it with the patch to allow this to be landed via the commit queue. Also, you need to update the ChangeLog tests line with how this is tested, or why it can't be tested.

> WebCore/rendering/InlineTextBox.cpp:46
> +#ifdef DISABLE_ROUNDING_HACKS

Looking at StringTruncator, which also uses TextRun::disableRoundingHacks(), the behaviour is triggered with a method argument with a default value. Is there a reason why we can't use the same approach here, to avoid ifdefs? Is it a lot of work to plumb such an argument through from WebKit? I've CC'ed hyatt and aroben who seemed to have worked on this code.

If we do use an ifdef, I think it should probably be a WTF USE macro.

> WebCore/rendering/InlineTextBox.cpp:47
> +static void disableRoundingForTextControls(const InlineTextBox* box, TextRun& tr)

Use complete words for variable names

Created attachment 75830 [details]
new patch taking previous advice into consideration

Although the changes look fine to me, and build in my normal environment, running build-webkit fails, but I don't understand the output:

Checking Dependencies...
** BUILD FAILED **

The following build commands failed:
JavaScriptCore:
	Ld /WebKitForPatches/WebKitBuild/Release/JavaScriptCore.framework/Versions/A/JavaScriptCore normal i386
jsc:
	Ld /WebKitForPatches/WebKitBuild/Release/jsc normal i386
testapi:
	Ld /WebKitForPatches/WebKitBuild/Release/testapi normal i386
(3 failures)

Created attachment 75924 [details]
patch

[Early Warning System Bot]

Attachment 75924 [details] did not build on qt:
Build output: http://queues.webkit.org/results/6771127

Created attachment 76121 [details]
patch

[Steve Block]

Comment on attachment 76121 [details]
patch

This looks reasonable to me. Would be good to get a review from aroben or hyatt before submitting.

Also, it looks like you need to rebase your patch so the try-bots can process it.

[Adam Roben (:aroben)]

I don't know enough about this code to know whether this is the right approach. Hyatt should probably comment.

Created attachment 76226 [details]
patch updated to tip of tree

Created attachment 76227 [details]
patch updated to tip of tree

[Steve Block]

Comment on attachment 76227 [details]
patch updated to tip of tree

Missing InlineTextBox.cpp

Created attachment 76380 [details]
patch

[Steve Block]

Comment on attachment 76380 [details]
patch

LGTM, hyatt can you take a look?

[Dave Hyatt]

Comment on attachment 76380 [details]
patch

I really don't think of this as a RenderTheme setting.  I just think it's a Setting.  That way it will be simple and non-virtual too.

It's not ok to add a member to InlineTextBoxes for this either.  That's too much of a memory hit.

There's gotta be a way to avoid patching all of the text functions like that too.

[Dave Hyatt]

I don't really get how you can disable rounding hacks without everything breaking.  Are you ok with mismatches in rendering when inlines like <spans> are used?  I just want to make sure you're aware of what breaks if you do this.

I just want to disable the rounding hacks for text controls.  This is because we overlay a text input view from our view system, and I want it to line up with the text on the web page.  The text doesn't line up after spaces though, due to the rounding hacks.

[Dave Hyatt]

I think the best way to accomplish this is to actually add a new CSS property for it.... probably something like -webkit-text-rounding: auto | integral | subpixel.  Then you can set that in your Android UA sheet for text controls.

The WebCore code can then be patched to just respect the CSS property.

Not sure if others agree with this approach, but that's probably how I would do it.  The CSS property support could even just be ifdefed out for platforms that don't care (similar to the Dashboard region CSS property).

I am not sure I agree with introducing a CSS property that inevitably results in rendering errors, but I guess it’s fine if ports that don’t want the broken behavior can disable it.

(In reply to comment #20)
> I think the best way to accomplish this is to actually add a new CSS property for it.... probably something like -webkit-text-rounding: auto | integral | subpixel.  Then you can set that in your Android UA sheet for text controls.
> 
> The WebCore code can then be patched to just respect the CSS property.
> 
> Not sure if others agree with this approach, but that's probably how I would do it.  The CSS property support could even just be ifdefed out for platforms that don't care (similar to the Dashboard region CSS property).

Thanks for the advice, Dave.

(In reply to comment #21)
> I am not sure I agree with introducing a CSS property that inevitably results in rendering errors, but I guess it’s fine if ports that don’t want the broken behavior can disable it.

What "rendering errors" will I run into?  I know there will be errors when text changes styles, for links etc, but I am only trying to remove the rounding hacks inside text controls (<input> and <textarea>), where there won't be any links.  Am I missing something?

Thanks,

(In reply to comment #22)
> (In reply to comment #21)
> > I am not sure I agree with introducing a CSS property that inevitably results in rendering errors, but I guess it’s fine if ports that don’t want the broken behavior can disable it.
> 
> What "rendering errors" will I run into?  I know there will be errors when text changes styles, for links etc, 

Anywhere you end up with more than one text box on the line. In addition to the above, this includes bidirectional text and collapsed whitespace.

> but I am only trying to remove the rounding hacks inside text controls (<input> and <textarea>), where there won't be any links.  Am I missing something?

So the CSS property could only apply to those elements?

(In reply to comment #23)
> So the CSS property could only apply to those elements?

I am not sure how to do that using a CSS property, but yes, my goal is to only do this for <input> and <textarea>

[Dave Hyatt]

See

https://bugs.webkit.org/show_bug.cgi?id=54244

Rounding hacks will be dying soon, so can probably just close this bug or dup it to that.

[Eric Seidel (no email)]

Android Chromium is the new future.