On Tue, 24 Aug 2021 22:12:01 GMT, Jose Pereda <jper...@openjdk.org> wrote:
>> Currently, `WebPage` has already a public `setBackgroundColor()` method, but
>> the class is not public. Therefore, public API is needed in `WebView` to
>> allow developers access to it.
>>
>> In line with the `fontSmoothingType` property, this PR provides public
>> support for setting the background color of a WebPage, by adding a
>> `pageFill` property, and a CSR is required.
>>
>> The color for the background, that can be opaque, transparent or with any
>> level of opacity, can be set via code or via CSS using `-fx-page-fill`.
>>
>> Unit tests and a system test are provided.
>
> Jose Pereda has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Update cssref.html
I left a couple more comments on the API docs, and a few comments / questions
on the implementation. I've tested this on Windows, but still need to test on
the other two platforms.
modules/javafx.graphics/src/main/java/com/sun/prism/es2/ES2Graphics.java line
70:
> 68: context.updateCompositeMode(CompositeMode.CLEAR);
> 69: Paint oldPaint = getPaint();
> 70: setPaint(Color.TRANSPARENT); // any color will do...
Is this change necessary? If so, then the comment is probably wrong.
modules/javafx.web/src/main/java/com/sun/javafx/webkit/prism/WCGraphicsPrismContext.java
line 459:
> 457: public void setClip(WCRectangle c) {
> 458: if (!isOpaque) {
> 459: clearRect((int)c.getX(), (int)c.getY(),
Are you sure that there are no ill effects from clearing the rectangle every
time a clip is set on a non-opaque context? This seems like a surprising side
effect.
modules/javafx.web/src/main/java/com/sun/webkit/WebPage.java line 615:
> 613: twkSetTransparent(frameID, isBackgroundTransparent());
> 614: twkSetBackgroundColor(frameID, backgroundColor);
> 615: repaintAll();
Is this needed unconditionally? Or only when the background is not opaque?
modules/javafx.web/src/main/java/com/sun/webkit/WebPage.java line 637:
> 635: twkSetBackgroundColor(frameID, backgroundColor);
> 636: }
> 637: repaintAll();
Same question as above.
modules/javafx.web/src/main/java/javafx/scene/web/WebView.java line 703:
> 701: * Specifies the background color of the web page.
> 702: *
> 703: * With this property, the WebView control's background
I recommend code style for `WebView`.
modules/javafx.web/src/main/java/javafx/scene/web/WebView.java line 707:
> 705: * level of transparency.
> 706: *
> 707: * However, if the HTML content being loaded set its own
Typo: set --> sets
modules/javafx.web/src/main/java/javafx/scene/web/WebView.java line 708:
> 706: *
> 707: * However, if the HTML content being loaded set its own
> 708: * background color, it will take precedence.
Maybe say the "that color" (instead of "it") will take precedence?
modules/javafx.web/src/main/java/javafx/scene/web/WebView.java line 733:
> 731: Color color = get();
> 732: page.setBackgroundColor(color != null ?
> color.hashCode() :
> 733: DEFAULT_PAGE_FILL.hashCode());
This is relying on an undocumented, implementation detail. The current
implementation of `Color::hashCode` happens to do what you want, but it is not
specified. It seems safer to create a utility method to do this.
-------------
PR: https://git.openjdk.java.net/jfx/pull/563
