8354795: DialogPane show details button wipes out base style class "hyperlink" #1779
Conversation
|
👋 Welcome back jhendrikx! A progress list of the required criteria for merging this PR into |
|
❗ This change is not yet ready to be integrated. |
openjdk
bot
changed the title
|
/reviewers 2 |
|
@kevinrushforth |
| detailsButton.setText(isExpanded ? lessText : moreText); | ||
| detailsButton.getStyleClass().setAll("details-button", (isExpanded ? "less" : "more")); //$NON-NLS-1$ //$NON-NLS-2$ //$NON-NLS-3$ | ||
| }; | ||
| styleClasses.add("details-button"); //$NON-NLS-1$ |
There was a problem hiding this comment.
do we use these //$NON-NLS-1$ in jfx?
There was a problem hiding this comment.
I do not know if we do, as there is also non-public code that I don't have access to. I've kept them as-is to make sure the change is focused and doesn't break anything else.
If I had to guess then probably not, if you can ask around and find out I'll be happy to remove them.
There was a problem hiding this comment.
I'll double check and will let you know tomorrow.
A code search shows that these are only used in 5 classes: ButtonBar, ButtonBarSkin, Dialog, DialogPane, Properties, so chances are it's a digital fossil.
There was a problem hiding this comment.
I can confirm that we don't use these //$NON-NLS-* comments in javaFX.
<rant>
These comments were invented as a way to solve the localization problem. The problem is real, but the "solution" was exactly the opposite. Instead of marking the strings that don't need to be localized, what should have happened is to mark the string that need to be localized - with the information containing the context!
example:
// verb, as in "execute this test"
String run = "Run";What I've done with some other projects was
String run = TXT.get("Class.LocalizedResourceID.verb, as in execute this test", "Run");</rant>
There was a problem hiding this comment.
Okay, I can remove them.
There was a problem hiding this comment.
line 822 should be
styleClasses.setAll("details-button");
|
An SCCE would have been nice, to help with the review/testing. edit: alternatively, I'll add the (missing) Dialog/DialogPane page to the monkey tester. |
If you still need one, I can provide a bit of code. It basically is just changing the hyperlink style, and then seeing it doesn't affect the Alert dialog when it has an extended area. After the change, it should affect it. I did include a screenshot -- I may find more of these bugs as I continue developing with modena.css with dark theme colors :) |
Thanks, that's ok, I would rather add the Dialog page to the MT, as it will allow for more extensive testing. |
|
I've added the Dialog page to the monkey tester Looking at the stock behavior, I don't really see the problem. The .details-button should not look a hyperlink (despite being implemented using Hyperlink control), so the code seems to be correct in that it replaces the hyperlink style with something else. What we probably need for the dark mode is to update the stylesheet as you described in the ticket. What do you think? |
|
|
I think it should look like a hyperlink, as the choices of potential clickable controls that users will recognize as such is limited; it can be either a button, checkbox or hyperlink. You can't just define something new and expect users to understand it at a glance; and why even do so for an alert dialog? This is clearly intended to mimic other standard alert dialogs that use the same kind of button to display details. There are so many things dependent on the base style in modena, not having a base style is highly unusual and in this case IMHO a clear mistake in how the styles are being applied. It just wasn't caught in testing as the default color matches close enough, but becomes rather obvious when changing the base colors significantly. |
It might look like a hyperlink, but it has a different semantics (semantics of a hyperlink is to navigate to some other place). When application wants to change the hyperlink styling, we don't want this to propagate to unrelated controls such as dialog buttons, or we get this: |
|
In Windows, and also our color chooser, hyperlinks are used with some regularity to avoid a button where it would look out of place -- in small windows, buttons often are seen as a "closing" action, which show details clearly is not intended to be. The Windows copy dialog for example has a show details text that's clickable, and when keyboard focused displays the classic hyperlink dashed rectangular border. I've looked in FX code, and wiping out the base style is AFAICS not done anywhere. There are of course many cases of If this indeed was the intent, then it would have made much more sense to not wipe out the base style, and only make adjustments specifically for hyperlinks that are also styled with |
|
It seems a bit unusual to remove the default style class of a control. If you use a If you don't want your control to be hyperlinky (and also not buttony), the cleaner solution would be to not use |
not necessarily, if you want all the functionality but different visuals. BTW, the circular button in the DialogPane looks nothing like other modena buttons. I suspect the code was transplanted from some other project. May be the DialogPane should have used the TitledPane for the expandable content, to be visually consistent with the rest of javaFX? In any case, I think it is wrong to retain the hyperlink style in this case. What I agree with is that it should not use So my suggestion would be:
|
Yes, but the only thing |
agree. 2014 was like, ten thousand years ago ;-) |
So... you want me to add more styles so it then ends up looking like a correctly colored hyperlink? Wouldn't it be easier to do this the other way around, leave the hyperlink style, then apply any specific styling that is needed to make a details-button? |
No. I would like to retain the existing functionality where the styles alternate between If you look at the existing code, the setAll() in line 825 gets invoked at the moment the details button is created because the listener gets called in line 829. So basically, your new code needs one change in (new) line 822:
the rest is good. |
Then we can close this, as that's what the original code does already. |
Exactly (but I like the changes you made to add/remove "more"/"less" styles, since they won't erase any styles that the application might add during construction). Let me ask about the original problem, which I read is difficulty to implement the dark theme. Should we make any changes to |
The details button is private and not documented, so it is hard to reach, and even if you could it would be unsupported.
The only change needed is to not wipe out the hyperlink style, then it will correctly follow the Modena specified colors which can be set to support a dark theme, see here how I do that: .root {
-fx-accent: #165693;
-fx-focus-color: -fx-accent;
-fx-base: #16181c;
-fx-background: #22252b;
-fx-control-inner-background: derive(-fx-base, 20%);
-fx-control-inner-background-alt: -fx-control-inner-background;
-fx-outer-border: derive(-fx-color, 23%);
-fx-selection-bar-non-focused: -fx-base;
/* Lighter than -fx-background and used to provide a small highlight when
* needed on top of -fx-background. This is never a shadow in Modena but
* keep -fx-shadow-highlight-color name to be compatible with Caspian.
*/
-fx-shadow-highlight-color: ladder(
-fx-background,
rgba(0,0,0,0.07) 0%,
rgba(0,0,0,0.07) 20%,
rgba(0,0,0,0.07) 70%,
rgba(0,0,0,0.7) 90%,
rgba(0,0,0,0.75) 100%
);
/* A gradient that goes from a bit lighter than -fx-color on the top to
* a little darker at the bottom. Typically is the third color in the
* -fx-background-color list as a thin highlight inside the outer border.
* Insets are typically 1.
*/
-fx-inner-border: linear-gradient(to bottom,
ladder(
-fx-color,
derive(-fx-color,-30%) 0%,
derive(-fx-color,-20%) 40%,
derive(-fx-color,-25%) 60%,
derive(-fx-color,-55%) 80%,
derive(-fx-color,-55%) 90%,
derive(-fx-color,-75%) 100%
),
ladder(
-fx-color,
derive(-fx-color,-20%) 0%,
derive(-fx-color,-10%) 20%,
derive(-fx-color,-5%) 40%,
derive(-fx-color,2%) 60%,
derive(-fx-color,5%) 100%
));
}This can be combined with a native API call (on Windows) to give your Window a dark title bar as well (if you want that code, let me know). Because the style is wiped out however, I have to work-around it and copy the hyperlink styles specifically for details-button, like so: /* copied from .hyperlink styles for each pseudo state */
.dialog-pane > .button-bar > .container > .details-button {
-fx-text-fill: -fx-accent;
}
.dialog-pane > .button-bar > .container > .details-button:armed,
.dialog-pane > .button-bar > .container > .details-button:visited,
.dialog-pane > .button-bar > .container > .details-button:hover:armed {
-fx-text-fill: -fx-text-background-color;
} |
it is not private: the method that creates it in the DialogPane:817 is protected, so the application can override it and get a pointer to the button, set styles, etc.
|
Adding these styles to |
Sorry, I missed that. So its returning a
Unfortunately, I think we're going in circles now. I've already given my opinion on this several times and how to most correctly resolve this. Anyone else have an opinion on how this should be resolved? |
The "show details" hyperlink button in an alert dialog that has an expandable detail area wipes out its base style class by overwriting all styles. This means styling in modena.css that targets
.hyperlinkis no longer applied, like the default text fill colors.The culprit is this code in DialogPane:
Here it uses
setAllto set styles, wiping out the default.hyperlinkstyle from "Hyperlink detailsButton = new HyperLink()"Progress
Issue
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/1779/head:pull/1779$ git checkout pull/1779Update a local copy of the PR:
$ git checkout pull/1779$ git pull https://git.openjdk.org/jfx.git pull/1779/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 1779View PR using the GUI difftool:
$ git pr show -t 1779Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/1779.diff
Using Webrev
Link to Webrev Comment