Ticket #353 (closed defect: fixed)

Opened 4 years ago

Last modified 19 months ago

Javascript should not be required for basic usage.

Reported by: Olivier Borowski <olivier.borowski@…> Owned by: DarTar
Priority: normal Milestone: 1.1.6.6
Component: handlers Version: 1.1.6.2
Severity: normal Keywords: JavaScript, forms, edit, acls
Cc:

Description (last modified by DotMG) (diff)

Wikka should work with a Javascript disabled browser. For example, there are Javascript actions in forms which can be easily replaced with PHP.

Subtask: #641.

Attachments

edit.php.patch Download (2.3 KB) - added by Olivier Borowski <olivier.borowski@…> 4 years ago.
Patch removing javascript call for "Cancel" button
acls.php.patch Download (0.8 KB) - added by Olivier Borowski <olivier.borowski@…> 4 years ago.
Patch removing javascript call for "Cancel" button

Change History

Changed 4 years ago by Olivier Borowski <olivier.borowski@…>

Patch removing javascript call for "Cancel" button

Changed 4 years ago by Olivier Borowski <olivier.borowski@…>

Patch removing javascript call for "Cancel" button

  Changed 4 years ago by DarTar

  • owner changed from unassigned to DarTar
  • status changed from new to assigned
  • version set to 1.1.6.2
  • milestone set to 1.1.7

Olivier,

you are perfectly right - we are progressively cleaning-up Wikka from unnecessary use of JS. Thanks for contributing the patches.

  Changed 4 years ago by Olivier Borowski <olivier.borowski@…>

We have to remove from header.php : 

$message = $this->GetRedirectMessage();
<body <?php echo $message ? "onload=\"alert('".$message."');\" " : "" ?> >

Could be replaced by a message like "revisioninfo", or error messages displayed in edit.php

If redirectmessages are only used when redirecting to a page containing show.php, we could simply add after "revisioninfo" message (~ line 76) : 

$message = $this->GetRedirectMessage();
if ($message)
{
	echo '<div class="messageClassName">'.$message.'</div>';
}

  Changed 4 years ago by JavaWoman

  • keywords JavaScript, added; Javascript, removed

Changing keyword so it's properly spelled (helps with queries ;))

  Changed 4 years ago by JavaWoman

  • keywords forms, added; form, removed

  Changed 4 years ago by JavaWoman

See also #312

  Changed 4 years ago by JavaWoman

JavaScript button for Logout in usersettings replaced by normal button in [330]; this was necessary to get rid of $_REQUEST for #312.

  Changed 3 years ago by DotMG

  • description modified (diff)

(change in description: added subtask #641)

  Changed 2 years ago by JavaWoman

  • milestone changed from 1.1.7 to 1.1.6.6

I really don't see why the milestone should be 1.1.7 on this, given that it makes the applications unusable for people who cannot use JavaScript. Worse, since this was reported for 1.1.6.2, by 1.1.6.5 we now have new JavaScript-dependent things like Cancel buttons that are easily avoidable. So 1.1.6.5 has JavaScript-dependent Cancel buttons in:

  • adminpages action
  • adminusers action
  • acls handler
  • delete handler
  • revisions handler

We should turn this around and make 1.1.6.6 the milestone, and port to 1.7 when making a change. Hopefully that will avoid introducing new ones, too. There really is no reason to continue to break the app for some 5-10% of end users.

... changing milestone! Let's backport what's already in trunk, and fix what isn't fixed yet.

  Changed 2 years ago by DarTar

Be my guest, but see my note on comment:ticket:122:6

  Changed 20 months ago by DarTar

(In [1271]) Preliminary implementation of JS-less system messages, as per Olivier's suggestion in http://wush.net/trac/wikka/ticket/353#comment:2. This patch attempts to unify the look of all system messages, but needs extensive testing as it may break the layout (it replaces previous occurrences of em.error and em.success and messages generated via the Redirect() method), refs #353 and #784

  Changed 20 months ago by DarTar

Possible issues after [1271]:

  • Redirect() must be able to specify the system message class as an extra parameter, i.e. error, success or notification. Currently it alwasy uses .success.
  • The current implementation only supports one system message per page, this can be a problem where the .error class is used in multiple places. The layout of /referrers, for one, gets messed up for this reason if no referrer is available for the current page.
  • If we like the idea of displaying system messages in the top right corner of the page div with the default layout, we may opt for a less intrusive position:fixed attribute instead of the current position:absolute (keeping in mind browser compatibility).

  Changed 20 months ago by DarTar

(In [1272]) System messages, minor layout change (using position:fixed), refs #353

  Changed 20 months ago by DarTar

(In [1273]) System messages, fixing layout, refs #353

  Changed 20 months ago by DarTar

(In [1274]) Partially reverting CSS changes for redirect messages. Unifying the styling of redirect and standard system messages creates a number of issues with the layout, so I'm keeping em.error and em.success for standard inline messages and div.success for redirect messages. This assumes that a redirect message is displayed only after an operation was successfully completed. refs #353

  Changed 20 months ago by BrianKoontz

(In [1279]) Removed JS dependencies for "Cancel" buttons. Refs #353.

  Changed 20 months ago by BrianKoontz

(In [1280]) Ported the "Cancel" button changes to trunk. Refs #353.

  Changed 20 months ago by DarTar

tested all modified cancel buttons, works for me

  Changed 20 months ago by DarTar

(In [1288]) Fixing syntax error introduced in [1280] (thanks Eclipse!), refs #353

  Changed 19 months ago by DarTar

(In [1308]) removing obsolete todo from phpdoc header, refs #353

  Changed 19 months ago by DarTar

It looks like, apart from places where JS is strictly required (e.g. edit toolbar), it has been replaced elsewhere. The only exception is the installer, where JS is still used to check input.

  Changed 19 months ago by BrianKoontz

(In [1310]) Removed JS dependencies in installer. Refs #353.

  Changed 19 months ago by BrianKoontz

  • status changed from accepted to testing

Just noticed that input field values aren't preserved when errors are generated. Also, the <font> tags need to be changed to access the appropriate CSS selector for error messages.

  Changed 19 months ago by DarTar

(In [1312]) Preliminary implementation of JS-less input validation in setup (needs to be extended to all required fields that can be modified via the form), importing CSS selectors for error/confirmation messages from main stylesheet (may need to replace the existing .ok and .failed used in later steps by the installer), refs #353

follow-up: ↓ 25   Changed 19 months ago by BrianKoontz

A couple of suggestions for [1312]:

  • Use <font>...</font> tags for error messages, or we need to fix the problem that still manifests itself with regards to the CSS not being accessed/loaded during installation (see #664).
  • Remove the break statements from the big switch, and process all of the fields at one time, rather than one at a time (might make for a frustrating install otherwise)

in reply to: ↑ 24   Changed 19 months ago by DarTar

Replying to BrianKoontz:

* Use <font>...</font> tags for error messages, or we need to fix the problem that still manifests itself with regards to the CSS not being accessed/loaded during installation (see #664).

that doesn't look like a solution, <font> is deprecated in XHTML, the right solution would be to embed the CSS directly in each file.

* Remove the break statements from the big switch, and process all of the fields at one time, rather than one at a time (might make for a frustrating install otherwise)

I agree, but my goal was to fix two problems with the previous implementation: (1) error messages should not be displayed before the form is submitted, (2) we should distinguish between the case where a field has been left empty and a field has an invalid value

  Changed 19 months ago by BrianKoontz

that doesn't look like a solution, <font> is deprecated in XHTML, the right solution would be to embed the CSS directly in each file.

Then we should consider option 2.

I agree, but my goal was to fix two problems with the previous implementation: (1) error messages should not be displayed before the form is submitted, (2) we > should distinguish between the case where a field has been left empty and a field has an invalid value

(1) was working in the original implementation. There is a test case at the start of the code to see if the session has started. If not, then no errors should be generated. Re (2), given the logical construct required to test both for empty fields and invalid values, an if/else flow might be preferable over a switch.

  Changed 19 months ago by DarTar

(In [1316]) JS-less input validation in setup, enforcing required fields and adding minimal constraints on the value of root_page (which was never enforced until 1.1.6.5!) - please test extensively (fresh installs and upgrades), refs #353

  Changed 19 months ago by DarTar

(In [1317]) JS-less input validation (more explicit instructions on valid root_page), refs #353

  Changed 19 months ago by DarTar

Brian, I reverted to if/else and (hopefully) covered all required fields. I also realized that in spite of what we say in the installer, we have never actually enforced the format of root_page. We say it should be formatted as a WikiName but we don't check for this! This is incidentally the reason why tags such as "home" work as root pages (my personal page uses this).

Discussing with Nils, we found a compromise between backwards-compatibility and minimal constraints to avoid breaking the installation (e.g. spaces should not be allowed and special characters raise encoding issues).

Please test extensively and try to break the installation/upgrade as bad as you can :)

  Changed 19 months ago by BrianKoontz

Version upgrade testing:

1.1.6.0 -> 1.1.6.6: success 1.1.6.1 -> 1.1.6.6: success 1.1.6.2 -> 1.1.6.6: success 1.1.6.3 -> 1.1.6.6: success 1.1.6.4 -> 1.1.6.6: success 1.1.6.5 -> 1.1.6.6: success

  Changed 19 months ago by BrianKoontz

Install testing:

  • CSS formatting: Fail (recommend using <font></font> tags for error output)
  • Required input fields empty: Success
  • Missing MySQL hostname, database name, username: Success
  • root_page too short: Success
  • non-CamelCase root_page: Fail (is this even a valid issue?)
  • Malformed admin name: Success
  • Password length < 5: Success
  • Password with no confirm: Success
  • Password mismatch: Success
  • Matching passwords: Success
  • Malformed adminstrator e-mail: Success
  • All fields valid, transition to install page: Success
  • Initial configuration - directory not writable: Success
  • Make directory writable and try again: Success
  • Install with mod_rewrite disabled: Success

  Changed 19 months ago by DarTar

(In [1319]) Embedding installer stylesheet in header.php to prevent URL rewrite issues, refs #664 and #353)

follow-up: ↓ 36   Changed 19 months ago by DarTar

Brian, [1319] should address the CSS issue, can you please confirm? Apart from this:

  • I spotted another problem when you reload the installer, the setup skips automatically to step 2, reporting that it was impossible to connect to MySQL. I suspect this is related to the session.
  • I don't understand: non-CamelCase root_page: Fail - it should allow tags such as "home" as it did prior to 1.1.6.6, can you confirm?

follow-up: ↓ 35   Changed 19 months ago by BrianKoontz

I don't understand: non-CamelCase root_page: Fail - it should allow tags such as "home" as it did prior to 1.1.6.6, can you confirm?

Ah, so that's allowable. Then it's a non-issue, and the test case is invalid. I'd edit the comment if I could!

I spotted another problem when you reload the installer, the setup skips automatically to step 2, reporting that it was impossible to connect to MySQL. I suspect this is related to the session.

I'm assuming you mean that if the initial connection attempt fails, and you attempt to go back to the step 1, you're redirected to step 2. I'll try to recreate.

in reply to: ↑ 34 ; follow-up: ↓ 38   Changed 19 months ago by DarTar

I'm assuming you mean that if the initial connection attempt fails, and you attempt to go back to the step 1, you're redirected to step 2. I'll try to recreate.

no, I mean that if you refresh the initial setup screen in the browser (without submitting the form), you get step 2 instead of step 1 as you would normally expect.

in reply to: ↑ 33   Changed 19 months ago by DarTar

[1319] should address the CSS issue

to be precise, this actually fixes the CSS issue, but the logo in the header may still break if the installer is loaded from the wrong URL.

  Changed 19 months ago by BrianKoontz

(In [1320]) Only redirect to install page in response to a POST action on the page (prevents reloads from redirecting). Refs #353.

in reply to: ↑ 35   Changed 19 months ago by BrianKoontz

Replying to DarTar:

I'm assuming you mean that if the initial connection attempt fails, and you attempt to go back to the step 1, you're redirected to step 2. I'll try to recreate.

no, I mean that if you refresh the initial setup screen in the browser (without submitting the form), you get step 2 instead of step 1 as you would normally expect.

Dario, please try to see if you can recreate this. Redirects should now only occur in response to a POST action on the page.

BTW, CSS looks great. The non-display of the logo is minor and really not worth holding things up at this point IMO.

  Changed 19 months ago by DarTar

(In [1321]) CSS image replacement to avoid errors when images are not accessible, added check on base_url and other minor changes, refs #664 and #353

  Changed 19 months ago by DarTar

Brian, [1320] fixed the problem, thanks. I tweaked the CSS in [1321] so if the logo is not loaded a simple text heading is displayed.

follow-up: ↓ 42   Changed 19 months ago by DotMG

If the DB parameters entered are invalid, Testing MySQL connection settings... FAILED with no back button

in reply to: ↑ 41   Changed 19 months ago by BrianKoontz

Replying to DotMG:

If the DB parameters entered are invalid, Testing MySQL connection settings... FAILED with no back button

The problem here is that there's no way to test the return value of test(...), so it's impossible to determine whether or not the page ended in an error. I propose simply displaying a message advising the user to click the browser's back button to correct errors on the previous page. See commit note below...

  Changed 19 months ago by BrianKoontz

(In [1322]) Removed display of error message on first load of default page; generate error message on install page if any test fails. Refs #353.

  Changed 19 months ago by DarTar

(In [1323]) [m] styling of error messages, refs #353

  Changed 19 months ago by DarTar

(In [1324]) [m] heading number, refs #353

  Changed 19 months ago by DarTar

Brian, works great - I made some minor styling adjustments, I think we are ready to roll this out.

  Changed 19 months ago by BrianKoontz

  • status changed from testing to commit

Changes tested through [1324].

  Changed 19 months ago by BrianKoontz

  • status changed from commit to closed
  • resolution set to fixed
Note: See TracTickets for help on using tickets.