Re: WICKET-6725 styling of hidden elements: no vote yet
Hi, as you know I am a big fan of HTML5 and was experimenting a lot with it, I would also vote to stay with a core css and clean it up, because of a better backward compatibility. flex is used a LOT in modern layouts and would break the other option. kind regards Tobias > Am 26.02.2020 um 11:31 schrieb Martijn Dashorst : > > On Wed, Feb 26, 2020 at 11:15 AM Emond Papegaaij > wrote: > On Tue, Feb 25, 2020 at 10:54 PM Sven Meier wrote: > - it's a kitchen-sink for left-over styles (see .wicket--color-red) >>> >>> I agree that for this one Wicket can just add the CSS class (e.g. >>> wicket-feedback-indicator) on the HTML element and let the application >>> provide the CSS rules for it >> >> I'm ok with that. I only added that styling to be compatible with the >> old behavior, which was broken in my opinion anyway. >> > > As long as the default behavior is specified in the Wicket core css file, > I'm for it. .wicket--color-red is godawful so +1 for making it meaningful. > > IMO this means that we should move the styling of e.g. the feedback panel > [1] into the wicket-- fold. However, that is not something that is related > to CSP, so CSP should not be the driver (nor wait) for that. > > >>> An idea: >>> if CSP is disabled then Wicket can deliver the content of wicket-core.css >>> as inline CSS, i.e. . >>> This will keep the number of http requests the same as before. >> >> This already is an option now and doesn't depend on CSP being enabled >> or not. As long as the style element is rendered with a nonce, it will >> work. We can make the header contribution a bit more flexible with the >> following options: >> * inline wicket-core.css (or another stylesheet) >> * resource reference to wicket-core.css (or another stylesheet) >> * no core stylesheet at all >> > > AFAIR the extra request is normally not a problem as browsers multiplex > that on the HTTP/2 connection to the server, and the CSS is easily cached. > It might actually be a worse experience to inline the style than having it > as a separate resource. > > Martijn > > [1] > https://github.com/apache/wicket/blob/master/wicket-core/src/test/java/org/apache/wicket/markup/html/panel/FeedbackPanelTest_cssClasses_expected.html
[GitHub] [wicket] reiern70 commented on a change in pull request #410: [WICKET-6750] allow to abort currently executing request for a given …
reiern70 commented on a change in pull request #410: [WICKET-6750] allow to abort currently executing request for a given … URL: https://github.com/apache/wicket/pull/410#discussion_r384522325 ## File path: wicket-core/src/main/java/org/apache/wicket/ajax/res/js/wicket-ajax-jquery.js ## @@ -335,6 +335,58 @@ Wicket.ChannelManager.FunctionsExecuter = FunctionsExecuter; + /** +* Channel manager maintains a map of channels. Review comment: TODO: Change this. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [wicket] reiern70 commented on a change in pull request #410: [WICKET-6750] allow to abort currently executing request for a given …
reiern70 commented on a change in pull request #410: [WICKET-6750] allow to
abort currently executing request for a given …
URL: https://github.com/apache/wicket/pull/410#discussion_r384522677
##
File path:
wicket-core/src/main/java/org/apache/wicket/ajax/res/js/wicket-ajax-jquery.js
##
@@ -335,6 +335,58 @@
Wicket.ChannelManager.FunctionsExecuter = FunctionsExecuter;
+ /**
+* Channel manager maintains a map of channels.
+*/
+ Wicket.AjaxRequestMonitor = Wicket.Class.create();
+
+ Wicket.AjaxRequestMonitor.prototype = {
+ initialize: function () {
+ this.requests = [];
+ },
+
+ init: function() {
+ var requests = this.requests;
+ Wicket.Event.subscribe('/ajax/call/beforeSend',
+ function(jqEvent, attributes, jqXHR,
errorThrown, textStatus) {
+ requests [attributes.ch] = jqXHR;
+ }
+ );
+
+ Wicket.Event.subscribe('/ajax/call/complete',
+ function(jqEvent, attributes, jqXHR,
errorThrown, textStatus) {
+ delete requests[attributes.ch];
+ }
+ );
+ },
+
+ getReuest: function (channel) {
Review comment:
and this
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [wicket] reiern70 commented on a change in pull request #410: [WICKET-6750] allow to abort currently executing request for a given …
reiern70 commented on a change in pull request #410: [WICKET-6750] allow to abort currently executing request for a given … URL: https://github.com/apache/wicket/pull/410#discussion_r384522325 ## File path: wicket-core/src/main/java/org/apache/wicket/ajax/res/js/wicket-ajax-jquery.js ## @@ -335,6 +335,58 @@ Wicket.ChannelManager.FunctionsExecuter = FunctionsExecuter; + /** +* Channel manager maintains a map of channels. Review comment: Change this. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [wicket] reiern70 opened a new pull request #410: [WICKET-6750] allow to abort currently executing request for a given …
reiern70 opened a new pull request #410: [WICKET-6750] allow to abort currently executing request for a given … URL: https://github.com/apache/wicket/pull/410 allow to abort currently executing request for a given Ajax channel + example This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
Re: WICKET-6725 styling of hidden elements: no vote yet
On Wed, Feb 26, 2020 at 11:15 AM Emond Papegaaij wrote: > > On Tue, Feb 25, 2020 at 10:54 PM Sven Meier wrote: > > > - it's a kitchen-sink for left-over styles (see .wicket--color-red) > > > > I agree that for this one Wicket can just add the CSS class (e.g. > > wicket-feedback-indicator) on the HTML element and let the application > > provide the CSS rules for it > > I'm ok with that. I only added that styling to be compatible with the > old behavior, which was broken in my opinion anyway. > As long as the default behavior is specified in the Wicket core css file, I'm for it. .wicket--color-red is godawful so +1 for making it meaningful. IMO this means that we should move the styling of e.g. the feedback panel [1] into the wicket-- fold. However, that is not something that is related to CSP, so CSP should not be the driver (nor wait) for that. > > An idea: > > if CSP is disabled then Wicket can deliver the content of wicket-core.css > > as inline CSS, i.e. . > > This will keep the number of http requests the same as before. > > This already is an option now and doesn't depend on CSP being enabled > or not. As long as the style element is rendered with a nonce, it will > work. We can make the header contribution a bit more flexible with the > following options: > * inline wicket-core.css (or another stylesheet) > * resource reference to wicket-core.css (or another stylesheet) > * no core stylesheet at all > AFAIR the extra request is normally not a problem as browsers multiplex that on the HTTP/2 connection to the server, and the CSS is easily cached. It might actually be a worse experience to inline the style than having it as a separate resource. Martijn [1] https://github.com/apache/wicket/blob/master/wicket-core/src/test/java/org/apache/wicket/markup/html/panel/FeedbackPanelTest_cssClasses_expected.html
Re: WICKET-6725 styling of hidden elements: no vote yet
On Tue, Feb 25, 2020 at 9:54 PM Sven Meier wrote: > [] leave as is with .wicket--hidden & wicket-core.css > > [] use HTML5 "hidden" attribute instead > While it is true that Wicket hasn't depended on a CSS file for its own use, it has been dependent on its own styles, spread out through our code in odd ways. The fact that we now have to own up to this by having to ship a stylesheet file of our own after 15 years, sounds more like 15 years of neglect and harassment of our users than an actual achievement. I consider not having a wicket stylesheet file a bug, not a feature. Having an actual Wicket css file means that our styles are *finally* documented and available for our users to accommodate, rather than strewn out through our code base and hidden in style attributes, only to be discovered through perusing the generated markup or ample browsing through java code. This is a great benefit: want to know what Wicket uses for styling? Here's the file! Furthermore, the wicket-core css file can be easily disabled if one desires so (then you have add your own implementations of those classes to your own css file), or overridden (e.g. wicket-bootstrap can provide its own core css file). And we provide the default template as well... This doesn't mean that I want us to ship a full bootstrap/material like CSS styling with Wicket, but rather only those parts that ensure that applications keep working. When something as simple as using flex or display:block on a div breaks the hidden attribute [1] we should not depend on it working. Telling folks to 'just add some arbitrary css to your styling to fix this attribute so some parts of your page remain invisible', is not a suitable substitute for providing our own css. Martijn [1] https://meowni.ca/hidden.is.a.lie.html
Re: WICKET-6725 styling of hidden elements: no vote yet
On Wed, Feb 26, 2020 at 10:53 AM Martin Grigorov wrote:
> I am also not big fan of CSP but users ask for it and I see no other way
> but to move all inline styles in such .css resource
IMHO CSP is one of the changes to the web application ecosystem that
Wicket will need an answer on to stay relevant in the upcoming years.
Web application development is changing at a very rapid pace and
Wicket will need to incorporate those changes or face becoming
obsolete.
> On Tue, Feb 25, 2020 at 10:54 PM Sven Meier wrote:
> > - it's a kitchen-sink for left-over styles (see .wicket--color-red)
>
> I agree that for this one Wicket can just add the CSS class (e.g.
> wicket-feedback-indicator) on the HTML element and let the application
> provide the CSS rules for it
I'm ok with that. I only added that styling to be compatible with the
old behavior, which was broken in my opinion anyway.
> >
< cut use hidden attribute >
> >
> I don't agree here.
> If each and every developer should make sure that Wicket's placeholder tags
> should stay invisible then the framework does not do its job.
> Often when we see that something is too common we actually add it to the
> framework. Here you ask to do the reverse - remove the support and ask the
> developers to deal with it.
> The article you linked to explains it well that "hidden" attribute does not
> really work. CSS libraries which provide reset rules like div { display:
> block; } will make the life of the developer miserable.
I totally agree with Martin. Using the hidden attribute puts the
responsibility with the developer where this should be on the
framework. The hidden attribute just doesn't work.
> An idea:
> if CSP is disabled then Wicket can deliver the content of wicket-core.css
> as inline CSS, i.e. .
> This will keep the number of http requests the same as before.
This already is an option now and doesn't depend on CSP being enabled
or not. As long as the style element is rendered with a nonce, it will
work. We can make the header contribution a bit more flexible with the
following options:
* inline wicket-core.css (or another stylesheet)
* resource reference to wicket-core.css (or another stylesheet)
* no core stylesheet at all
Best regards,
Emond
[GitHub] [wicket] reiern70 commented on a change in pull request #409: [WICKET-6750] allow to abort currently executing AJAX request
reiern70 commented on a change in pull request #409: [WICKET-6750] allow to
abort currently executing AJAX request
URL: https://github.com/apache/wicket/pull/409#discussion_r384385873
##
File path:
wicket-core/src/main/java/org/apache/wicket/ajax/res/js/wicket-ajax-jquery.js
##
@@ -547,13 +576,29 @@
*/
ajax: function (attrs) {
this._initializeDefaults(attrs);
-
var res = Wicket.channelManager.schedule(attrs.ch,
Wicket.bind(function () {
this.doAjax(attrs);
}, this));
return res !== null ? res: true;
},
+ /**
+* Aborts current AJAX request, if any is running, for default
channel.
+* WARNING! Mind that this does not implies and server
immediately will know about
Review comment:
@andruhon thanks for your comment.
I have reworded it like above. Hope is is more clear now.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
Re: WICKET-6725 styling of hidden elements: no vote yet
Hi Sven,
On Tue, Feb 25, 2020 at 10:54 PM Sven Meier wrote:
> Hi all,
>
> we have a disagreement on how to style hidden elements in Wicket 9.x.
>
> Due to the new CSP support we can no longer use inline styling to hide
> elements.
> WICKET-6725 introduces new CSS classes and a file wicket-core.css.
>
> I don't think this is a good approach:
>
> - it adds a CSS file that is referenced by each page (after Wicket doing
> fine without it for 15 years)
>
I am also not big fan of CSP but users ask for it and I see no other way
but to move all inline styles in such .css resource
> - the CSS is a mingle-mangle of out-of-date stylings (see
> .wicket--hidden-fields)
>
What do you suggest to use instead ?
> - it's a kitchen-sink for left-over styles (see .wicket--color-red)
>
I agree that for this one Wicket can just add the CSS class (e.g.
wicket-feedback-indicator) on the HTML element and let the application
provide the CSS rules for it
> - it introduces a new class naming scheme not used anywhere else (wicket--)
>
We need this "namespace" to avoid clashes with the applications' styles. I
see nothing bad in this one.
>
> IMHO we should remove that file again (and the required infrastructure
> in ResourceSettings/WebApplication) and just
> use the HTML5 "hidden" attribute instead, whenever we want to hide
> something (Component, Form, ...).
This "just works" in all browsers and is semantically correct. It has
> one caveat when an application's CSS changes the default styling of
> hidden elements (see
> https://css-tricks.com/the-hidden-attribute-is-visibly-weak), but that's
> in the responsibility of the application developer.
>
I don't agree here.
If each and every developer should make sure that Wicket's placeholder tags
should stay invisible then the framework does not do its job.
Often when we see that something is too common we actually add it to the
framework. Here you ask to do the reverse - remove the support and ask the
developers to deal with it.
The article you linked to explains it well that "hidden" attribute does not
really work. CSS libraries which provide reset rules like div { display:
block; } will make the life of the developer miserable.
> AjaxIndicatorAppender can just render a CSS class and leave the styling
> to the application developer, nobody will be happy with the default
> "red" anyway.
>
+1 on this ^^
>
> Thus I'll be starting a vote in the next days with the following two
> options:
>
> [] leave as is with .wicket--hidden & wicket-core.css
>
> [] use HTML5 "hidden" attribute instead
>
> This isn't the vote yet, it's just the announcement.
> Maybe others see a third (forth?) option or want to raise their concerns
> first.
>
An idea:
if CSP is disabled then Wicket can deliver the content of wicket-core.css
as inline CSS, i.e. .
This will keep the number of http requests the same as before.
Martin
>
> Sven
>
>
>
[GitHub] [wicket] reiern70 commented on a change in pull request #409: [WICKET-6750] allow to abort currently executing AJAX request
reiern70 commented on a change in pull request #409: [WICKET-6750] allow to
abort currently executing AJAX request
URL: https://github.com/apache/wicket/pull/409#discussion_r384373648
##
File path:
wicket-core/src/main/java/org/apache/wicket/ajax/res/js/wicket-ajax-jquery.js
##
@@ -547,13 +576,29 @@
*/
ajax: function (attrs) {
this._initializeDefaults(attrs);
-
var res = Wicket.channelManager.schedule(attrs.ch,
Wicket.bind(function () {
this.doAjax(attrs);
}, this));
return res !== null ? res: true;
},
+ /**
+* Aborts current AJAX request, if any is running, for default
channel.
+* WARNING! Mind that this does not implies and server
immediately will know about
Review comment:
> This does not imply that the server will immediately know about that? Is
it what you're trying to tell?
>
> I think this comment needs rewording.
What I mean is "there is not reliable way to tell the server stop what I
asked you to do". In case of upload stream will be broken and upload halted
with a server side exception.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
Re: WICKET-6725 styling of hidden elements: no vote yet
Hi, Right now I have no enough knowledge to vote in this feature. One thing I didn't like, and I already mentioned it before, is some of us were waiting for 9.x to be released some time ago (at least a few months ago I was preparing some branch of our application and ported it to 9.x, after asking about release plans) and all of the sudden this feature is introduced and all sub-frameworks depending on Wicket will have to be adapted. Thus in practice the release of 9.x. by itself, with new CSS feature, will not bring any immediate value to many users as this will break most existing applications. On Wed, Feb 26, 2020 at 10:34 AM Maxim Solodovnik wrote: > [+] leave as is with .wicket--hidden & wicket-core.css > > IMO we should sheep the version which will work as expected out-of-the-box > According to my tests `hidden` attribute doesn't work (even `display: > flex` breaks it) > > On Wed, 26 Feb 2020 at 15:22, Andrea Del Bene > wrote: > > > > +1 to vote. I find your concerns legitimate > > > > On Tue, Feb 25, 2020 at 9:54 PM Sven Meier wrote: > > > > > Hi all, > > > > > > we have a disagreement on how to style hidden elements in Wicket 9.x. > > > > > > Due to the new CSP support we can no longer use inline styling to hide > > > elements. > > > WICKET-6725 introduces new CSS classes and a file wicket-core.css. > > > > > > I don't think this is a good approach: > > > > > > - it adds a CSS file that is referenced by each page (after Wicket > doing > > > fine without it for 15 years) > > > - the CSS is a mingle-mangle of out-of-date stylings (see > > > .wicket--hidden-fields) > > > - it's a kitchen-sink for left-over styles (see .wicket--color-red) > > > - it introduces a new class naming scheme not used anywhere else > (wicket--) > > > > > > IMHO we should remove that file again (and the required infrastructure > > > in ResourceSettings/WebApplication) and just > > > use the HTML5 "hidden" attribute instead, whenever we want to hide > > > something (Component, Form, ...). > > > This "just works" in all browsers and is semantically correct. It has > > > one caveat when an application's CSS changes the default styling of > > > hidden elements (see > > > https://css-tricks.com/the-hidden-attribute-is-visibly-weak), but > that's > > > in the responsibility of the application developer. > > > AjaxIndicatorAppender can just render a CSS class and leave the styling > > > to the application developer, nobody will be happy with the default > > > "red" anyway. > > > > > > Thus I'll be starting a vote in the next days with the following two > > > options: > > > > > > [] leave as is with .wicket--hidden & wicket-core.css > > > > > > [] use HTML5 "hidden" attribute instead > > > > > > This isn't the vote yet, it's just the announcement. > > > Maybe others see a third (forth?) option or want to raise their > concerns > > > first. > > > > > > Sven > > > > > > > > > > > > > -- > > Andrea Del Bene. > > Apache Wicket committer. > > > > -- > WBR > Maxim aka solomax > -- Regards - Ernesto Reinaldo Barreiro
Re: WICKET-6725 styling of hidden elements: no vote yet
[+] leave as is with .wicket--hidden & wicket-core.css IMO we should sheep the version which will work as expected out-of-the-box According to my tests `hidden` attribute doesn't work (even `display: flex` breaks it) On Wed, 26 Feb 2020 at 15:22, Andrea Del Bene wrote: > > +1 to vote. I find your concerns legitimate > > On Tue, Feb 25, 2020 at 9:54 PM Sven Meier wrote: > > > Hi all, > > > > we have a disagreement on how to style hidden elements in Wicket 9.x. > > > > Due to the new CSP support we can no longer use inline styling to hide > > elements. > > WICKET-6725 introduces new CSS classes and a file wicket-core.css. > > > > I don't think this is a good approach: > > > > - it adds a CSS file that is referenced by each page (after Wicket doing > > fine without it for 15 years) > > - the CSS is a mingle-mangle of out-of-date stylings (see > > .wicket--hidden-fields) > > - it's a kitchen-sink for left-over styles (see .wicket--color-red) > > - it introduces a new class naming scheme not used anywhere else (wicket--) > > > > IMHO we should remove that file again (and the required infrastructure > > in ResourceSettings/WebApplication) and just > > use the HTML5 "hidden" attribute instead, whenever we want to hide > > something (Component, Form, ...). > > This "just works" in all browsers and is semantically correct. It has > > one caveat when an application's CSS changes the default styling of > > hidden elements (see > > https://css-tricks.com/the-hidden-attribute-is-visibly-weak), but that's > > in the responsibility of the application developer. > > AjaxIndicatorAppender can just render a CSS class and leave the styling > > to the application developer, nobody will be happy with the default > > "red" anyway. > > > > Thus I'll be starting a vote in the next days with the following two > > options: > > > > [] leave as is with .wicket--hidden & wicket-core.css > > > > [] use HTML5 "hidden" attribute instead > > > > This isn't the vote yet, it's just the announcement. > > Maybe others see a third (forth?) option or want to raise their concerns > > first. > > > > Sven > > > > > > > > -- > Andrea Del Bene. > Apache Wicket committer. -- WBR Maxim aka solomax
Re: WICKET-6725 styling of hidden elements: no vote yet
+1 to vote. I find your concerns legitimate On Tue, Feb 25, 2020 at 9:54 PM Sven Meier wrote: > Hi all, > > we have a disagreement on how to style hidden elements in Wicket 9.x. > > Due to the new CSP support we can no longer use inline styling to hide > elements. > WICKET-6725 introduces new CSS classes and a file wicket-core.css. > > I don't think this is a good approach: > > - it adds a CSS file that is referenced by each page (after Wicket doing > fine without it for 15 years) > - the CSS is a mingle-mangle of out-of-date stylings (see > .wicket--hidden-fields) > - it's a kitchen-sink for left-over styles (see .wicket--color-red) > - it introduces a new class naming scheme not used anywhere else (wicket--) > > IMHO we should remove that file again (and the required infrastructure > in ResourceSettings/WebApplication) and just > use the HTML5 "hidden" attribute instead, whenever we want to hide > something (Component, Form, ...). > This "just works" in all browsers and is semantically correct. It has > one caveat when an application's CSS changes the default styling of > hidden elements (see > https://css-tricks.com/the-hidden-attribute-is-visibly-weak), but that's > in the responsibility of the application developer. > AjaxIndicatorAppender can just render a CSS class and leave the styling > to the application developer, nobody will be happy with the default > "red" anyway. > > Thus I'll be starting a vote in the next days with the following two > options: > > [] leave as is with .wicket--hidden & wicket-core.css > > [] use HTML5 "hidden" attribute instead > > This isn't the vote yet, it's just the announcement. > Maybe others see a third (forth?) option or want to raise their concerns > first. > > Sven > > > -- Andrea Del Bene. Apache Wicket committer.
