Page MenuHomePhabricator

SPIKE verify that mobile preferences toggles aren't overwriting existing checkbox handlers [8hr]
Closed, ResolvedPublic

Description

It occurred to me that we should probably check to see what happens if a checkbox already has it's own custom onclick/onchange event handling. I think we'd end up paving right over it. I thought to create that scenario locally to see if we should check for it and return early (before replacing the checkbox), but I ran out of time.

Question:
how does our toggle code impact checkbox handlers? checkbox handlers are skipped when a user interacts with the toggle

Event Timeline

usana Cardenas Molinar9:11 AM
Search for the getPreferences hook implementation, search onClick in extension

I think this has been fairly well discussed, and believe that this is ready to be moved to the next vertical

Samwalton9-WMF renamed this task from SPIKE verify that mobile preferences toggles aren't overwriting existing checkbox handlers to SPIKE verify that mobile preferences toggles aren't overwriting existing checkbox handlers [8hr].Dec 8 2022, 6:34 PM

2 hours
Spend time looking at how onclick works in javascrpt world. Am looking at HTML DOM Element addEventListener() which may take care of the issue, but need to play around for a bit.

BTW, this is not adapted for screen readers

perhaps using blur or change rather than click

document.getElementById("Yadayadayada").addEventListener("blur", function(e) {
     console.log("It was clicked"); 
});

addEventListener() Syntax
Here's the syntax:

target.addEventListener(event, function, useCapture)

target: the HTML element you wish to add your event handler to. This element exists as part of the Document Object Model (DOM) and you may wish to learn about how to select a DOM element.
event: a string that specifies the name of the event. We already mentioned load and click events. For the curious, here's a full list of HTML DOM events.
function: specifies the function to run when the event is detected. This is the magic that can allow your web pages to change dynamically.
useCapture: an optional Boolean value (true or false) that specifies whether the event should be executed in the capturing or bubbling phase. In the case of nested HTML elements (such as an img within a div) with attached event handlers, this value determines which event gets executed first. By default, it's set to false which means that the innermost HTML event handler is executed first (bubbling phase).

Passing Event as a Parameter
Sometimes we may want to know more information about the event, such as what element was clicked. In this situation, we need to pass in an event parameter to our function.

This example shows how to obtain the element's id:

button.addEventListener('click', (e)=>{
  console.log(e.target.id)
})

Here the event parameter is a variable named e but it can be easily called anything else such as "event". This parameter is an object which contains various information about the event such as the target id.

Good point about accessibility. I think we're using onchange instead of onclick on the toggles so that it's agnostic to the interaction that caused the value to change. To bring this back to the preferences checkboxes: since we're already selecting the checkbox widgets in the insertToggles function, perhaps your next step could be to add an event handler there (to the checkbox widget and/or maybe to the checkbox input itself) instead of hiding the checkbox and then see what happens when you click the toggle.

@jsn.sherman good idea, kind of opposite of what i was thinking, but your suggestion may make it a lot quicker

well, getting no joy insofar as breaking the dang thing. - the section setting the value does not change the value, so I don't think there will be an issue. If a ticket comes out of this, it might be to add an event listener, but I am not sure that it will add anything to the code

		toggleSwitchWidget.on( 'change', function ( value ) {
				toggleSwitchWidget.setValue( value );
				checkboxInput.checked = value;
			} );

it could, on the way outer fringe

function insertToggles( checkboxes ) {
		Array.prototype.forEach.call( checkboxes, function ( checkboxWidget ) {

			var checkboxInput = checkboxWidget.querySelector( 'input' );
			checkboxWidget.addEventListener('click', function(){console.log('click'); } );
			checkboxInput.addEventListener('change', function(){console.log('change'); } );
			var toggleSwitchWidget = new OO.ui.ToggleSwitchWidget( {
				value: checkboxInput.checked,
				disabled: checkboxInput.disabled
			} );
			toggleSwitchWidget.on( 'change', function ( value ) {
				toggleSwitchWidget.setValue( value ); console.log( value );
				checkboxInput.checked = value;
			} );
			checkboxWidget.insertAdjacentElement( 'afterend', toggleSwitchWidget.$element[ 0 ] );
		//	checkboxWidget.classList.add( 'hidden' );
		} );
	}

Ellen, thanks for your work on this! I really appreciate the test code. I can see that you have findings, but I'm not clear on exactly how this can break from reading the comment. Could you share your findings in a little bit more detail? I can see that you added event listeners on the checkbox input and the widget that wraps around it. Could you share what happened with each of those? Eg. did they both (input and widget) get missed when changing the toggle, or did only one get missed?

Could you put your findings into the description as an answer to the question:

If so, how does our toggle code impact these handlers?

Feel free to mark

Is there wmf code with custom onClick handlers for checkboxes?

as N/A since we decided to skip it

If so, how does our toggle code impact these handlers?
toggle fires onchange not onclick
when the toggle is switched the checkbox changes also but the toggle doesn't change
when the checkbox is checked as expected

when changing toggle from false to true the value on change becomes true on toggle and on the hidden checkbox
when moving changed toggle from true to false the value becomes false on toggle and on the hidden checkbox

If the checkbox is checked (when exposed) the checkbox will show change, (checkboxWidget with event listener) BUT the click event will not fire

console log messages
on section display, nothing fires, but toggle is displayed, checkbox is not
clicking toggle to false- console.log displays 'value on change false'

  • the displayed hidden check box is also unchecked

clicking toggle to true- console.log displays 'value on change true'

  • the displayed hidden check box is also checked

now, the toggle has been set to true
when unchecking checkbox - console.log displays 'click' then 'change', the toggle stays in the false position, and the value does not change

checking checkbox - console.log displays 'click' then 'change', the toggle stays in the true position, and the value does not change

when toggle is set to true, the checkbox shows as checked, and the value is true, - console.log displays 'value on change true'
when toggle is set to false, the checkbox shows as not checked, and the value is false - console.log displays 'value on change false'

To sum it up, there is an infinitesimal chance there may be a conflict - I was not able to find a conflict, most checkboxes use the onclick, whereas this code is using onchange -

Is there wmf code with custom onClick handlers for checkboxes?
N/A

I am not 100% sure I understand, so I'm going to try some reflective reading and you can tell me if I got it right:
When you click a checkbox:

  • does console.log('change') code run?
  • does console.log('click') for its parent checkbox widget run?

from this statement:

when unchecking checkbox - console.log displays 'click' then 'change', the toggle stays in the false position, and the value does not change
checking checkbox - console.log displays 'click' then 'change', the toggle stays in the true position, and the value does not change

I think the you are saying the answer is yes to both; meaning you have a control that verifies that checkbox handlers work on their own.

When you click a toggle:

  • does console.log('change') code run for the checkbox the toggle is controlling?
  • does console.log('click') for the parent checkbox widget run?

from this statement:

when toggle is set to true, the checkbox shows as checked, and the value is true, - console.log displays 'value on change true'
when toggle is set to false, the checkbox shows as not checked, and the value is false - console.log displays 'value on change false'

I think you are saying the answer is no to both; meaning you have a test that shows that checkbox handlers are skipped when a user interacts with the toggle. That right there would be your finding, and the answer to the second question in the spike.

Do I have it right? If so, could you put your answer in the spike description?

Thanks for the investigative work on this! The above discussion was somewhat technical so I just want to clarify: Does this mean we have more work to do, or are there no issues here?

@Samwalton9 there is a potential issue here, for which I'll write up a backlog task to address.