Bug 227899 – Rename hasOverflowClip() to prepare for the real overflow:clip

• Component: CSS
• Status: RESOLVED FIXED

[Rob Buis]

Introduce isScrollContainer helper and start using it.

[Rob Buis]

Created attachment 433393 [details]
Patch

[Rob Buis]

Created attachment 433396 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 433396 [details]
Patch

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

> Source/WebCore/ChangeLog:14
> +        The overflow property decides whether the RenderObject
> +        is scrollable, isScrollContainer captures this.
> +        Right now isScrollContainer will only return false for
> +        overflow visible, but the upcoming overflow clip is
> +        also not scrollable.

Let's choose a good name for this.

isScrollContainer() is vague because the element might not be scrollable, and overflow:hidden is not user-scrollable, but is programmatically scrollable. I want "overflow" in the name, because it's about the overflow property.

We also need to decide if this helper will return true or false for overflow:clip, and if we need another helper for the visible/clip combination.

For the current behavior, I suggest:

hasNonVisibleOverflow()

For the future "potentially scrollable" case I suggest:

hasPotentiallyScrollableOverfow() or something.

[Rob Buis]

Created attachment 433435 [details]
Patch

[Rob Buis]

Comment on attachment 433396 [details]
Patch

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

>> Source/WebCore/ChangeLog:14
>> +        also not scrollable.
> 
> Let's choose a good name for this.
> 
> isScrollContainer() is vague because the element might not be scrollable, and overflow:hidden is not user-scrollable, but is programmatically scrollable. I want "overflow" in the name, because it's about the overflow property.
> 
> We also need to decide if this helper will return true or false for overflow:clip, and if we need another helper for the visible/clip combination.
> 
> For the current behavior, I suggest:
> 
> hasNonVisibleOverflow()
> 
> For the future "potentially scrollable" case I suggest:
> 
> hasPotentiallyScrollableOverfow() or something.

That makes sense. But hasNonVisibleOverflow would just be a rename of hasOverflowClip I think (chromium did this)?

I think it helps readibility and overflow: clip to add a method hasProgrammaticallyScrollableOverflow for programmatic scroll API, let me know what you think.

[Simon Fraser (smfr)]

(In reply to Rob Buis from comment #5)
> Comment on attachment 433396 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=433396&action=review
> 
> >> Source/WebCore/ChangeLog:14
> >> +        also not scrollable.
> > 
> > Let's choose a good name for this.
> > 
> > isScrollContainer() is vague because the element might not be scrollable, and overflow:hidden is not user-scrollable, but is programmatically scrollable. I want "overflow" in the name, because it's about the overflow property.
> > 
> > We also need to decide if this helper will return true or false for overflow:clip, and if we need another helper for the visible/clip combination.
> > 
> > For the current behavior, I suggest:
> > 
> > hasNonVisibleOverflow()
> > 
> > For the future "potentially scrollable" case I suggest:
> > 
> > hasPotentiallyScrollableOverfow() or something.
> 
> That makes sense. But hasNonVisibleOverflow would just be a rename of
> hasOverflowClip I think (chromium did this)?

That's the point!
 
> I think it helps readibility and overflow: clip to add a method
> hasProgrammaticallyScrollableOverflow for programmatic scroll API, let me
> know what you think.

But hasProgrammaticallyScrollableOverflow is also ambiguous: is it hasProgrammaticallyOrUserScrollableOverflow ?

[Rob Buis]

Created attachment 433446 [details]
Patch

[Rob Buis]

Comment on attachment 433396 [details]
Patch

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

>>>> Source/WebCore/ChangeLog:14
>>>> +        also not scrollable.
>>> 
>>> Let's choose a good name for this.
>>> 
>>> isScrollContainer() is vague because the element might not be scrollable, and overflow:hidden is not user-scrollable, but is programmatically scrollable. I want "overflow" in the name, because it's about the overflow property.
>>> 
>>> We also need to decide if this helper will return true or false for overflow:clip, and if we need another helper for the visible/clip combination.
>>> 
>>> For the current behavior, I suggest:
>>> 
>>> hasNonVisibleOverflow()
>>> 
>>> For the future "potentially scrollable" case I suggest:
>>> 
>>> hasPotentiallyScrollableOverfow() or something.
>> 
>> That makes sense. But hasNonVisibleOverflow would just be a rename of hasOverflowClip I think (chromium did this)?
>> 
>> I think it helps readibility and overflow: clip to add a method hasProgrammaticallyScrollableOverflow for programmatic scroll API, let me know what you think.
> 
> That's the point!

Oops, I thought somehow the bug was about adding a new method. Done.

[Rob Buis]

(In reply to Simon Fraser (smfr) from comment #6)
> > I think it helps readibility and overflow: clip to add a method
> > hasProgrammaticallyScrollableOverflow for programmatic scroll API, let me
> > know what you think.
> 
> But hasProgrammaticallyScrollableOverflow is also ambiguous: is it
> hasProgrammaticallyOrUserScrollableOverflow ?

My idea was to restrict that one to programmatic scrolling only. I am not sure yet if we need a hasUserScrollableOverflow. Both may be simple aliases though, anyway this is probably pretty minor, will have another look at that kind of naming tomorrow.

[EWS]

Committed r279918 (239667@main): <https://commits.webkit.org/239667@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 433446 [details].

[Radar WebKit Bug Importer]

<rdar://problem/80594572>