OPAC user notification preferences carrier dropdown issue

Bug #1710972 reported by Cesar V
8
This bug affects 1 person
Affects Status Importance Assigned to Milestone
Evergreen
Invalid
Undecided
Unassigned

Bug Description

Work for Bug 1098685 and related did not catch an html syntax error present in sms_carrier_selection.tt2
(http://git.evergreen-ils.org/?p=working/Evergreen.git;a=commitdiff;h=6c1b98b3b3b77338474a2cab95f606ae0b2f6602#patch1).
the <option> tag has BOOLEAN attribute "selected" that should be set to "true" for selection to work correctly.

The below branch fixes the issue.

Tags: opac
Revision history for this message
Cesar V (cesardv) wrote :
Revision history for this message
Chris Sharp (chrissharp123) wrote :

I can confirm that this fixes the issue specified here, and here's the signoff branch:

http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/csharp/lp1710972_opac_sms_carriers_dropdown

However, I do think the Notification Preferences UI needs the same sort of form validation that was introduced in the fixed in bug 1098685, so I'm going to open a new bug for that (for real this time :-)).

tags: added: signedoff
Revision history for this message
Chris Sharp (chrissharp123) wrote :

Bug 1711181 created to address the form validation issue. Thanks, Cesar!

Revision history for this message
Cesar V (cesardv) wrote :

Thanks Chris, sounds good.

I agree the myOPAC user Notification Preferences should have the same front-end JS validation behavior to make sure the user select their SMS carrier if SMS hold alerts are desired.

Revision history for this message
Galen Charlton (gmc) wrote :

Are you sure this a syntax error? And are there actually user-visible consequence?

I'm asking because the OPAC serves up HTML5, and per the spec "true" is expressly forbidden as a value:

https://www.w3.org/TR/html5/infrastructure.html#boolean-attributes

"A number of attributes are boolean attributes. The presence of a boolean attribute on an element represents the true value, and the absence of the attribute represents the false value.

If the attribute is present, its value must either be the empty string or a value that is an ASCII case-insensitive match for the attribute's canonical name, with no leading or trailing whitespace.

Note: The values "true" and "false" are not allowed on boolean attributes. To represent a false value, the attribute has to be omitted altogether."

Removing signoff tag.

tags: removed: signedoff
Revision history for this message
Cesar V (cesardv) wrote :

Galen, you're right the html5 spec does say it should be just the bool attribute "selected" in that option tag. It did seem to be behaving strange for me when I tested, I think my user's saved SMS carrier wouldn't display, and it would always default to the first one ("Alaska something") but perhaps that could have been a cache issue. Also selected='selected' looks wrong to me...

But anyhow, so to have it follow the spec, the "selected" tag should just look like this:

<option value='whatevs' selected>Text</option>

right?

I can amend my commit to reflect that

Revision history for this message
Galen Charlton (gmc) wrote :

selected="selected" is perfectly cromulant:

"... its value must either be the empty string or a __value that is an ASCII case-insensitive match for the attribute's canonical name, with no leading or trailing whitespace__."

and is used elsewhere in the OPAC templates. A bare "selected" is also fine, but it's not worth a patch changing to it in just one place.

Anyway, I'm not able to reproduce a bug: if I set a default mobile carrier in preferences, it gets pre-selected on the form. Consequently, I'm marking this one invalid; if you can get a problem to happen consistently, feel free to reopen this one.

tags: removed: pullrequest
Changed in evergreen:
status: New → Invalid
Revision history for this message
Cesar V (cesardv) wrote :

Sounds good. Sorry for the confusion ><

To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.