Bug 139946 – Fix typo in StyleResolveTree.cpp

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Maksim Kisilev]

In constructor of class CheckForVisibilityChangeOnRecalcStyle there is a line:
, m_previousImplicitVisibility(WKObservingContentChanges() && WKContentChange() != WKContentVisibilityChange ? elementImplicitVisibility(element) : VISIBLE)

Calling WKContentChange() will produce enum of type WKContentChange whose value will alwayse be not equal to WKContentVisibilityChange.
Probably author wanted to call WKObservedContentChange() instead of WKContentChange().

Line should be replace with:
, m_previousImplicitVisibility(WKObservingContentChanges() && WKObservedContentChange() != WKContentVisibilityChange ? elementImplicitVisibility(element) : VISIBLE)

[Maksim Kisilev]

Created attachment 243745 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 2 files


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

[Maksim Kisilev]

Created attachment 243746 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 243746 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        No new tests. Behaviour wasn't changed.

If there is no change in behavior, what is the rationale for making this code change?

[Maksim Kisilev]

(In reply to comment #4)
> Comment on attachment 243746 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=243746&action=review
> 
> > Source/WebCore/ChangeLog:8
> > +        No new tests. Behaviour wasn't changed.
> 
> If there is no change in behavior, what is the rationale for making this
> code change?

Final behavior wasn't changed. But code before change had meaningless check for:
WKContentChange() != WKContentVisibilityChange

[Brent Fulgham]

(In reply to comment #5)
> (In reply to comment #4)
> > Comment on attachment 243746 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=243746&action=review
> > 
> > > Source/WebCore/ChangeLog:8
> > > +        No new tests. Behaviour wasn't changed.
> > 
> > If there is no change in behavior, what is the rationale for making this
> > code change?
> 
> Final behavior wasn't changed. But code before change had meaningless check
> for:
> WKContentChange() != WKContentVisibilityChange

Yes, this code is wrong. WKContentChange() is not a function, it's a type -- so this is like saying "int() != WKContentVisibilityChange"

The correct method is WKObservedContentChange().

I'm not sure what impact this has in practice, but it should be corrected.

[Brent Fulgham]

Created attachment 273584 [details]
Patch

[Brent Fulgham]

I rebaselined Maksim's patch. Let's see what EWS says...

[Brent Fulgham]

EWS was happy with this change.

[Andy Estes]

Comment on attachment 273584 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        No new tests. Behaviour wasn't changed.

You state below that you actually expect some change in behavior, so I don't think you can claim nothing changed.

[Brent Fulgham]

I'm trying to see if I can figure out a test case for this.

This code is only used on iOS, and only triggers in the mouseMoved and 'handleSyntheticClick" event handler.

Businessinsider.com helpfully generates synthetic clicks for you, which shows the code getting hit. With the bug, we always use the "elementImplicitVisibility" path. This means that an element might stay hidden that (according to this code) should have been forced to be VISIBLE.

[Daniel Bates]

(In reply to comment #11)
> I'm trying to see if I can figure out a test case for this.
> 

You may find inspiration from looking at the tests included in <http://trac.webkit.org/changeset/194038> (bug #144451). In particular, see test <http://trac.webkit.org/browser/trunk/LayoutTests/fast/events/can-click-element-on-page-with-active-pseudo-class-and-search-field.html>. We may have additional tests that exercise the CheckForVisibilityChangeOnRecalcStyle code in <http://trac.webkit.org/browser/trunk/LayoutTests/platform/ios-simulator/ios>.

[Brent Fulgham]

Committed r198044: <http://trac.webkit.org/changeset/198044>