Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8090547: Allow for transparent backgrounds in WebView #563

Closed

Conversation

jperedadnr
Copy link
Collaborator

@jperedadnr jperedadnr commented Jul 2, 2021

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.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8090547: Allow for transparent backgrounds in WebView

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jfx pull/563/head:pull/563
$ git checkout pull/563

Update a local copy of the PR:
$ git checkout pull/563
$ git pull https://git.openjdk.java.net/jfx pull/563/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 563

View PR using the GUI difftool:
$ git pr show -t 563

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jfx/pull/563.diff

@bridgekeeper
Copy link

bridgekeeper bot commented Jul 2, 2021

👋 Welcome back jpereda! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot added the rfr Ready for review label Jul 2, 2021
@mlbridge
Copy link

mlbridge bot commented Jul 2, 2021

@kevinrushforth
Copy link
Member

/reviewers 2
/csr

@openjdk
Copy link

openjdk bot commented Jul 2, 2021

@kevinrushforth
The number of required reviews for this PR is now set to 2 (with at least 1 of role reviewers).

@openjdk openjdk bot added the csr Need approved CSR to integrate pull request label Jul 2, 2021
@openjdk
Copy link

openjdk bot commented Jul 2, 2021

@kevinrushforth has indicated that a compatibility and specification (CSR) request is needed for this pull request.
@jperedadnr please create a CSR request for issue JDK-8090547. This pull request cannot be integrated until the CSR request is approved.

Copy link
Member

@kevinrushforth kevinrushforth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This new API warrants a brief discussion on the mailing list prior to review. Some things to think about:

  • Should the new WebView property be a Color or should it be a Paint?

  • You propose to add a new CSS element, -fx-page-fill. Do we need it or can one of the existing elements work (e.g., -fx-background-color)? If we do end up adding a new element, you will need to document it in cssref.html.

*
* Default color: White
*
* @since JavaFX 17
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We no longer use JavaFX in the version for @since commands. And it's too late for 17, so this should be:

* @since 18

@mlbridge
Copy link

mlbridge bot commented Jul 2, 2021

Mailing list message from José Pereda on openjfx-dev:

* Should the new `WebView` property be a `Color` or should it be a

`Paint`?

WebPage.setBackgroundColor calls:

JNIEXPORT void JNICALL Java_com_sun_webkit_WebPage_twkSetBackgroundColor
(JNIEnv* env, jobject self, jlong pFrame, jint backgroundColor)

which only admits an int value for the background color (the hash value of
the color) that applies as:

frame->view()->setBaseBackgroundColor(...)

so I don't see a way to apply anything different than a plain color (with
any level of opacity) for the whole view.

For that reason, I didn't use Paint, but just Color.

* You propose to add a new CSS element, `-fx-page-fill`. Do we need it or

can one of the existing elements work (e.g., `-fx-background-color`)? If we
do end up adding a new element, you will need to document it in
`cssref.html`.

If we can't use Paint that also means that we can't use
Background::BACKGROUND_COLOR (and -fx-background-color). That's the reason
I'm proposing this new CSS element.

* @since 18

Ok, I'll fix that.

On Fri, Jul 2, 2021 at 2:50 PM Kevin Rushforth <kcr at openjdk.java.net> wrote:

--

1 similar comment
@mlbridge
Copy link

mlbridge bot commented Jul 2, 2021

Mailing list message from José Pereda on openjfx-dev:

* Should the new `WebView` property be a `Color` or should it be a

`Paint`?

WebPage.setBackgroundColor calls:

JNIEXPORT void JNICALL Java_com_sun_webkit_WebPage_twkSetBackgroundColor
(JNIEnv* env, jobject self, jlong pFrame, jint backgroundColor)

which only admits an int value for the background color (the hash value of
the color) that applies as:

frame->view()->setBaseBackgroundColor(...)

so I don't see a way to apply anything different than a plain color (with
any level of opacity) for the whole view.

For that reason, I didn't use Paint, but just Color.

* You propose to add a new CSS element, `-fx-page-fill`. Do we need it or

can one of the existing elements work (e.g., `-fx-background-color`)? If we
do end up adding a new element, you will need to document it in
`cssref.html`.

If we can't use Paint that also means that we can't use
Background::BACKGROUND_COLOR (and -fx-background-color). That's the reason
I'm proposing this new CSS element.

* @since 18

Ok, I'll fix that.

On Fri, Jul 2, 2021 at 2:50 PM Kevin Rushforth <kcr at openjdk.java.net> wrote:

--

@jperedadnr
Copy link
Collaborator Author

@mlbridge
Copy link

mlbridge bot commented Jul 5, 2021

Mailing list message from Johan Vos on openjfx-dev:

On Fri, Jul 2, 2021 at 2:50 PM Kevin Rushforth <kcr at openjdk.java.net> wrote:

On Fri, 2 Jul 2021 11:01:56 GMT, Jose Pereda <jpereda at 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.

This new API warrants a brief discussion on the mailing list prior to
review.

This is one of the old issues that keeps popping up regularly, and that
many developers expressed interest in. I think the approach of having a
`pageFill` similar to the `fontSmootingType` is a good approach. The main
thing I would be very careful about is not to introduce performance
regression in case no background is set. The current PR introduces a number
of points where additional computations are requested (e.g. `repaint`) but
these are guarded by checks that only pass when the background is set.

- Johan

@kevinrushforth kevinrushforth self-requested a review July 7, 2021 18:46
@micheljung
Copy link

micheljung commented Jul 8, 2021

I'll throw a party when this finally gets fixed! See also: javafxports/openjdk-jfx#277

Copy link
Member

@kevinrushforth kevinrushforth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The API for the new property looks fine. I left a couple comments on the javadoc.

You can create a Draft CSR when you get a chance. You still need to update cssref.html and that will need to be part of the CSR.

Comment on lines 701 to 702
* Specifies the background color of the webPage, allowing
* some or full transparency.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You might want to split this into two sentences, with the part about allowing for transparent being in the second sentence. Another thing you should indicate is how this interacts with the background color in the HTML file (I presume the one in the file takes precedence?). Also "web page" should be two words.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

* Specifies the background color of the webPage, allowing
* some or full transparency.
*
* Default color: White
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use an @defaultValue javadoc tag here:

    @defaultValue {@code Color.WHITE}

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@micheljung
Copy link

Since my comment may have gone unnoticed because I had to check the aggreement box first:

See javafxports/openjdk-jfx#277

@jperedadnr
Copy link
Collaborator Author

@kevinrushforth the CSR was already created here: https://bugs.openjdk.java.net/browse/JDK-8269848

Do I need to update its code with the latest changes of this PR?

As for the cssref.html update, do I add it as a commit to this PR, or as a new PR with the CSR id?

@kevinrushforth kevinrushforth self-requested a review August 23, 2021 15:35
@kevinrushforth
Copy link
Member

Do I need to update its code with the latest changes of this PR?

Yes, although you can "batch up" the changes if you like until the review is far enough along that the API is unlikely to change. One comment on the CSR is that the specification section is concerned with interface changes. So you can (and should) elide the body of any public methods and just leave the method signature and API docs. For properties, the private field is often included in the CSR, since that's what you hang the API docs on. For the styleable property, it should be sufficient to include the diffs for cssref.html once you have them (see below).

As for the cssref.html update, do I add it as a commit to this PR, or as a new PR with the CSR id?

Add it as a commit to this PR (btw, the JBS ID of the CSR is never used directly as an issue resolved by a PR; it is instead associated with a bug or enhancement that is).

@jperedadnr
Copy link
Collaborator Author

Ok, thanks for the clarifications. I've updated the CSR accordingly, and pushed the change for cssref.html.

Copy link
Member

@kevinrushforth kevinrushforth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@kevinrushforth kevinrushforth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just have a couple remaining comments.

Copy link
Member

@kevinrushforth kevinrushforth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left a few minor comments, but otherwise this all looks good to me.

Once you make the one requested change to remove the paragraph break, you can also update the CSR with that change and the two other changes I requested in the CSR, and then move the CSR to proposed.

@@ -93,6 +94,7 @@
private int width, height;

private int fontSmoothingType;
private int backgroundIntRgba = 0xFFFFFFFF;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add back the comment about this being Color.WHITE?


private static int getIntRgba(Color color) {
if (color == null) {
return -1;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe: return 0xFFFFFFFF; ? or else assign color = Color.WHITE; and fall through?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably better then to create a constant?

private static final int DEFAULT_BACKGROUND_INT_RGBA = 0xFFFFFFFF; // Color.WHITE

Comment on lines 705 to 707
* level of transparency.
*
* <p>However, if the HTML content being loaded sets its own
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked at the generated javadoc and this will read better without a paragraph break.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked at the generated javadoc and this will read better without a paragraph break.

When you remove the paragraph break, please also remove the blank line to make it clearer to future readers of the code that it is intended to be part of the same paragraph as the previous sentence.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Copy link
Collaborator Author

@jperedadnr jperedadnr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've also updated the CSR


private static int getIntRgba(Color color) {
if (color == null) {
return -1;
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably better then to create a constant?

private static final int DEFAULT_BACKGROUND_INT_RGBA = 0xFFFFFFFF; // Color.WHITE

Comment on lines 705 to 707
* level of transparency.
*
* <p>However, if the HTML content being loaded sets its own
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Copy link
Member

@kevinrushforth kevinrushforth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Latest changes look good. The new test fails for me on Linux, though. I tried it both on my Ubuntu 20.04 VM and on an older Ubuntu 16.0.4 physical machine.

I don't think it's just a test issue, either. When the OpenGL pipeline I get some odd looking ghosting around the text while the test is scrolling. Have you tested this on Linux?

@jperedadnr
Copy link
Collaborator Author

jperedadnr commented Sep 9, 2021

Yes, I have been testing on MacOS (11.5.1), Linux (Ubuntu 20.04) and Windows (10) with a manual test. I do a quick visual test with transparent, translucent and solid color, including scrolling and selecting some text.

However, the PageFillTest system test (which was actually based on this manual test) involves also taking snapshots and pixel comparison.

On MacOS and Windows it works fine. On Linux I see this "ghosting" while scrolling, but snapshots are taken on a steady position, and it shouldn't be there. What assertion fails for you?

Edit: I've done a few snapshots and now I see that the Linux font is taller and wider and the pixels that work fine on Mac/Windows are now failing on Linux. I'll see if I can fix that and use the same font.

@openjdk openjdk bot removed the csr Need approved CSR to integrate pull request label Sep 10, 2021
@jperedadnr
Copy link
Collaborator Author

@kevinrushforth It should be fixed now, the test should pass on the three platforms.

@kevinrushforth
Copy link
Member

I'll test on all three platforms and report back.

@kevinrushforth
Copy link
Member

The test now passes on one of my Linux machines, but I still see the smearing of the text when scrolling. I took the html file from your test and used it in a simple interactive program using a transparent background that will show the problem. If you run the test program on either Mac or Linux (using the es2 pipeline) and scroll the window, you will see that the scroll area isn't cleared. I found that if the color has any alpha > 0 it works correctly. This means that something is behaving differently with fully transparent (alpha == 0) in the ES2 pipeline. It might be related to why you needed to change the initial fill color for the ES2 pipeline.

SimpleWebViewTransp.java.txt

@jperedadnr
Copy link
Collaborator Author

Thanks for testing it, I can reproduce the issue indeed.

I had my interactive test too which doesn't fail with alpha = 0, and comparing both tests I realise now that in my case I was using a transparent VBox instead of a Group as a container for the WebView, and later on when I created the system test, I removed such container.

If you try:

webView.setPageFill(Color.TRANSPARENT);

var root = new VBox();
root.setStyle("-fx-background-color: transparent");
root.getChildren().add(webView);

that should work for you as well...

Obviously there is an issue with alpha = 0 in case we use Group or not container at all, vs when we use a Region.

The comment at Region::doComputeGeomBounds my be relevant though?

the geom bounds forms the basis of the dirty regions

@jperedadnr
Copy link
Collaborator Author

Somehow by accident I've found out that doing in WebPage::paint2GC:

                if (clip != null) {
                    if (isBackgroundColorTransparent()) {
+                        gc.clearRect(0, 0, 0, 0); // extra call to clear rect
                        gc.clearRect((int) clip.getX(), (int) clip.getY(), (int) clip.getWidth(), (int) clip.getHeight());
                    }
                    gc.setClip(clip);

fixes the issue for the Group test (same for no container case).

Ultimately, this is the same as calling twice ES2Graphics::clearQuad.

After some testing, I modified this method to get it working with the expected single call with just this change:

        CompositeMode mode = getCompositeMode();
-        // set the blend mode to CLEAR
-        context.updateCompositeMode(CompositeMode.CLEAR);
         Paint oldPaint = getPaint();
         setPaint(Color.BLACK); // any color will do...
         fillQuad(x1, y1, x2, y2);
+        // set the blend mode to CLEAR
+        context.updateCompositeMode(CompositeMode.CLEAR);
        context.flushVertexBuffer();

which seems to indicate that fillQuad requires SRC_OVER and we can use the original black color, and only before flushing we set the CLEAR mode.

Does this make sense?

@kevinrushforth
Copy link
Member

Somehow by accident I've found out that doing in WebPage::paint2GC:

+                        gc.clearRect(0, 0, 0, 0); // extra call to clear rect

fixes the issue for the Group test (same for no container case).

Interesting.

After some testing, I modified this method to get it working with the expected single call with just this change:

-        // set the blend mode to CLEAR
-        context.updateCompositeMode(CompositeMode.CLEAR);
         Paint oldPaint = getPaint();
         setPaint(Color.BLACK); // any color will do...
         fillQuad(x1, y1, x2, y2);
+        // set the blend mode to CLEAR
+        context.updateCompositeMode(CompositeMode.CLEAR);
        context.flushVertexBuffer();

which seems to indicate that fillQuad requires SRC_OVER and we can use the original black color, and only before flushing we set the CLEAR mode.

Even more interesting.

Does this make sense?

No, it doesn't make sense, since setting the mode to CLEAR before drawing the quad is the right order. The fact that it matters indicates that something very odd is happening with state management.

Your experiment was the clue that allowed me to figure out the problem. The problem is that the call to context.updateCompositeMode is simply the wrong thing to do. The right call, which is what the D3D pipeline is doing, is setCompositeMode. The former does not update the state so any other method that validates the state will use the wrong composite mode. It's amazing to me that this went undetected for this long, and that it hasn't caused other problems.

Here is the proposed change:

         CompositeMode mode = getCompositeMode();
         // set the blend mode to CLEAR
-        context.updateCompositeMode(CompositeMode.CLEAR);
+        setCompositeMode(CompositeMode.CLEAR);
         Paint oldPaint = getPaint();
         setPaint(Color.BLACK); // any color will do...
         fillQuad(x1, y1, x2, y2);
         context.flushVertexBuffer();
         setPaint(oldPaint);
         // restore default blend mode
-        context.updateCompositeMode(mode);
+        setCompositeMode(mode);

This will need to be well tested on Linux and Mac (a full set of Robot tests).

@kevinrushforth
Copy link
Member

I verified that the above change works, and doesn't cause any regressions that I can see.

@jperedadnr
Copy link
Collaborator Author

Great finding, thanks for clarifying it. I've tested it successfully too. I'll commit it then.

Copy link
Member

@kevinrushforth kevinrushforth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.

Copy link
Member

@arapte arapte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good to me too.

@openjdk
Copy link

openjdk bot commented Sep 21, 2021

@jperedadnr This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8090547: Allow for transparent backgrounds in WebView

Reviewed-by: kcr, arapte

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 69 new commits pushed to the master branch:

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready Ready to be integrated label Sep 21, 2021
@jperedadnr
Copy link
Collaborator Author

/integrate

@openjdk
Copy link

openjdk bot commented Sep 21, 2021

Going to push as commit f59a057.
Since your change was applied there have been 69 commits pushed to the master branch:

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot closed this Sep 21, 2021
@openjdk openjdk bot added integrated Pull request has been integrated and removed ready Ready to be integrated rfr Ready for review labels Sep 21, 2021
@openjdk
Copy link

openjdk bot commented Sep 21, 2021

@jperedadnr Pushed as commit f59a057.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integrated Pull request has been integrated
5 participants