Bug 22759 – dropdown menu disappears on mouse-over

• Component: New Bugs
• Status: RESOLVED FIXED

[jasneet]

I Steps:
Go to http://www.ebuyer.com/

II Issue:
Hover over the main navigation menus ( Home, Computers, Sound & Vision, etc)
Then move the mouse down and try to select something from drop down list 
that appears. Dropdown menu disappears : unable to select

III Conclusion:
there is a gap between the main menu item and the sub-menu box, that is the cause of the issue. Chrome/Safari should render the sub-menu box more higher to remove the gap, like what IE/Firefox does.

IV Other Browsers:
IE7: ok
FF3: ok

V Nightly tested: 39088

Bug in Chromium : http://code.google.com/p/chromium/issues/detail?id=2019

[jasneet]

Created attachment 25891 [details]
reduced testcase

[tvk]

Following calculations calculate the expected results for the
reduction HTML file attached to the issue.

Root ListItem vertical size, "RLITotal" = 0.6em top padding + text
line height + 0.7em bottom padding, where

Text line height = 16px * 70/100 * 1.5 = 16.8

So "RLITotal" = 0.6 * 16 * 70 / 100 + 16.8 + 0.7 * 16 * 70 / 100
                    = 6.72 + 16.8 + 7.84
                    = 31.36

The absolute positioned popup list's vertical position = 2.8em
                                                       = 2.8 * 16 * 70 /100
                                                       = 31.36

Though there is no gap between root element and popup, in Chrome we
see some gap.

What happens inside code during parsing the HTML file is that floating
pointing number gets converted to integer without rounding off. So
6.72 becomes 6, 7.84 becomes 7 and 16.8 becomes 16 resulting in total
for "RLITotal" as 29.

And absolute vertical location for popup, 31.36 becomes 31.

Clearly two pixel gap results because of this rounding off behaviour.

Inside following function,

int computedLineHeight() const
{
        Length lh = lineHeight();
        // Negative value means the line height is not set.  Use the
font's built-in spacing.
        if (lh.isNegative())
            return font().lineSpacing();
        if (lh.isPercent())
            return lh.calcMinValue(fontSize(), true);
        return lh.value();
}

The line, "return lh.calcMinValue(fontSize(), true);" was changed by
adding true as second parameter, which lets the calcMinValue to round
off the result to nearest integer. So in above example 16.82 becomes
17.

But that will not be sufficient because we'll still have 1 pixel gap.

So adding 0.5 to result before casting it to integer in the following
function fixes the issue. This function is a generic function used
during HTML/CSS file parsing.

int CSSPrimitiveValue::computeLengthIntForLength(RenderStyle* style,
double multiplier)
{
    double result = computeLengthDouble(style, multiplier);

    // This conversion is imprecise, often resulting in values of,
e.g., 44.99998.  We
    // need to go ahead and round if we're really close to the next
integer value.
    result += result < 0 ? -0.01 : +0.01;

    if (result > intMaxForLength || result < intMinForLength)
        return 0;
    return static_cast<int>(result);
}

I do not know why only 0.01 was added above, what happens if we add 0.5 is to be understood. 
I am not sure how to work further on this issue to take it to some sort of conclusion. Any help would be appreciated.

Created attachment 73438 [details]
Patch

Comment on attachment 73438 [details]
Patch

This is based on the solution outlined by tvk in https://bugs.webkit.org/show_bug.cgi?id=22759#c2

It looks like a bunch of layout tests will need to be rebaselined.  The standard procedure is to mark all the failing tests in the test_expectations files and then run the rebaselining script as described in http://trac.webkit.org/wiki/Rebaseline ?  This is my first webkit patch so I want to make sure I do everything right.

[Darin Adler]

Comment on attachment 73438 [details]
Patch

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

> WebCore/css/CSSPrimitiveValue.h:46
> -    value += (value < 0) ? -0.01 : +0.01;
> +    value += (value < 0) ? -0.5 : +0.5;

I’m pretty sure this is not acceptable. We discussed this when originally writing the code here and tried it and saw problems. Did you run regression tests? What tests did this affect?

By the way, if we *do* want this rounding, then I think we want to use the appropriate rounding function, roundf or round, rather than rolling our own with +=.

Comment on attachment 73438 [details]
Patch

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

>> WebCore/css/CSSPrimitiveValue.h:46
>> +    value += (value < 0) ? -0.5 : +0.5;
> 
> I’m pretty sure this is not acceptable. We discussed this when originally writing the code here and tried it and saw problems. Did you run regression tests? What tests did this affect?
> 
> By the way, if we *do* want this rounding, then I think we want to use the appropriate rounding function, roundf or round, rather than rolling our own with +=.

There were ~170 unexpected failures.  Most fail because of pixel differences unrelated to the tests' substance or are improvements on the existing results.  Two failing tests were on point though: svg/custom/fractional-rects and fast/dom/length-attribute-mapping.  They both expect lengths specified as floats to be rounded down.  Firefox seems to agree with them.

I'm going to work on this a bit more to get those two tests to pass along with the one I'm introducing here.  Thanks for the feedback.

On round(f): agreed.

Created attachment 76229 [details]
patch

Don't round lengths if specified units are pixels.  Round everything else.

I think this is more correct than the current behaviour but it moves a lot of stuff by 1 pixel.  Including h2 and h3 elements that are seemingly ubiquitous in pixel tests, of which approximately 200 are affected.  I've looked at all of them except some of the mozilla table tests.  None indicate that this patch breaks anything, they're all either improvements (minority) or noise (majority).

[WebKit Review Bot]

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

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/css1/units/rounding-expected.txt', u'LayoutTests/css1/units/rounding.html', u'WebCore/css/CSSPrimitiveValue.cpp']" exit_code: 1
WebCore/css/CSSPrimitiveValue.cpp:42:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/css/CSSPrimitiveValue.cpp:435:  rounded_result is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/css/CSSPrimitiveValue.cpp:438:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 3 in 3 files


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

Created attachment 76248 [details]
patch

Don't round lengths if specified units are pixels.  Round everything else.

I think this is more correct than the current behaviour but it moves a lot of stuff by 1 pixel.  Including h2 and h3 elements that are seemingly ubiquitous in pixel tests, of which approximately 200 are affected.  I've looked at all of them except some of the mozilla table tests.  None indicate that this patch breaks anything, they're all either improvements (minority) or noise (majority).

(same substance as attachment 76229 [details], fixed style errors)

[Adam Barth]

Comment on attachment 76248 [details]
patch

Thanks for the patch, but all patches need a ChangeLog.  See http://webkit.org/coding/contributing.html

Created attachment 76255 [details]
patch

Don't round lengths if specified units are pixels.  Round everything else.

I think this is more correct than the current behaviour but it moves a lot of stuff by 1 pixel.  Including h2 and h3 elements that are seemingly ubiquitous in pixel tests, of which approximately 200 are affected.  I've looked at all of them except some of the mozilla table tests.  None indicate that this patch breaks anything, they're all either improvements (minority) or noise (majority).

(same substance as attachment 76248 [details], now includes changelog)

(In reply to comment #5)
> (From update of attachment 73438 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=73438&action=review
> 
> > WebCore/css/CSSPrimitiveValue.h:46
> > -    value += (value < 0) ? -0.01 : +0.01;
> > +    value += (value < 0) ? -0.5 : +0.5;
> 
> I’m pretty sure this is not acceptable. We discussed this when originally writing the code here and tried it and saw problems. Did you run regression tests? What tests did this affect?

Darin A, who might remember what those problems were?  I didn't notice anything clearly problematic in the layout tests when they were run with the most recent patch.

(I added you to the cc list of this bug because I'm not sure if you'll see this message otherwise.  Apologies in advance if that's a faux pas.)

[Darin Adler]

(In reply to comment #12)
> (I added you to the cc list of this bug because I'm not sure if you'll see this message otherwise.  Apologies in advance if that's a faux pas.)

It’s always fine to cc me on any bug.

[Darin Adler]

(In reply to comment #12)
> (In reply to comment #5)
> > > WebCore/css/CSSPrimitiveValue.h:46
> > > -    value += (value < 0) ? -0.01 : +0.01;
> > > +    value += (value < 0) ? -0.5 : +0.5;
> > 
> > I’m pretty sure this is not acceptable. We discussed this when originally writing the code here and tried it and saw problems. Did you run regression tests? What tests did this affect?
> 
> Darin A, who might remember what those problems were?  I didn't notice anything clearly problematic in the layout tests when they were run with the most recent patch.

Not sure. Maybe Hyatt, maybe Mitz. Sorry I can’t remember.

[Darin Adler]

Comment on attachment 76255 [details]
patch

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

Change looks fine, but review- for a slight bit of unneeded work due to the way the code is structured.

> WebCore/css/CSSPrimitiveValue.cpp:439
> +    double roundedResult = result;
> +    if (shouldRound)
> +        roundedResult = round(result);
>      if (!applyZoomMultiplier || multiplier == 1.0)
> -        return result;
> -     
> +        return roundedResult;

Why compute roundedResult outside the if statement where it’s being returned? That seems like extra work for no benefit. I would write:

    if (!applyZoomMultiplier || multiplier == 1.0)
        return shouldRound ? round(result) : result;

Or the equivalent with two if statements.

Comment on attachment 76255 [details]
patch

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

>> WebCore/css/CSSPrimitiveValue.cpp:439
>> +        return roundedResult;
> 
> Why compute roundedResult outside the if statement where it’s being returned? That seems like extra work for no benefit. I would write:
> 
>     if (!applyZoomMultiplier || multiplier == 1.0)
>         return shouldRound ? round(result) : result;
> 
> Or the equivalent with two if statements.

Agreed.  It was an artifact of some debug code.

Created attachment 78083 [details]
Patch updated to address Darin's comment.

[David Grogan]

Created attachment 84112 [details]
Patch

[David Grogan]

Comment on attachment 84112 [details]
Patch

Ping and patch updated to latest code

[David Levin]

Comment on attachment 78083 [details]
Patch updated to address Darin's comment.

It looks like a more recent version has been uploaded.

[David Levin]

Comment on attachment 84112 [details]
Patch

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

> Source/WebCore/css/CSSPrimitiveValue.cpp:450
>      double zoomedResult = result * multiplier;

I'm not the right person to review this but speaking as someone who may read this code in the future, I find this patch confusing.

Specifically, there is a variable "shouldRound" and it isn't followed in all return statements. Is it a bug that this isn't done on on the final return "return zoomedResult;"?

[Eric Seidel (no email)]

Comment on attachment 84112 [details]
Patch

I agree with levin.  We need to make thsi consistent.

[David Grogan]

This is fixed in the chromium port because it uses subpixel layout.

[Alexey Proskuryakov]

> This is fixed in the chromium port because it uses subpixel layout.

But the reduction still fails in WebKit ToT, because it doesn't. Not sure if the bug needs to be reopened though, because the site changed, and works fine now.