Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

When creating new Health Facility, date picker shows over search box #6486

Closed
mrjones-plip opened this issue Jun 11, 2020 · 16 comments
Closed
Assignees
Labels
Type: Bug Fix something that isn't working as intended

Comments

@mrjones-plip
Copy link
Contributor

mrjones-plip commented Jun 11, 2020

I was setting up my dev environment and went to set up a new health facility. When selecting the age of the new person that's created in the process, I see a seemingly random datepicker appear over the "search everything" box in the upper left of the browser.

To Reproduce
Assuming a totally clean install using localhost:5988, Steps to reproduce the behavior:

  1. Go to People and click on "New health facility"
  2. On the form select "Create a new person"
  3. Enter a name and then click then enter an age using the date picker which is correctly positioned. We'll call this the primary datepicker.
  4. Click into the year part of the "age" input field. Change the year to a new, valid year
  5. click to the right of the input filed to close the datepicker
  6. A secondary datepicker is shown, incorrectly positioned above the search box

A more simple way of reproducing, again assuming an entirely clean install:

  1. Go to People and click on "New health facility"
  2. On the form select "Create a new person"
  3. click on the lablel for age ( question or-branch non-select invalid-required )
  4. A secondary datepicker is shown, incorrectly positioned above the search box

This may be systemic: when creating a user when creating a New Area and New Household, the same problem manifested.

Expected behavior
The primary datepicker closes and no secondary datepicker is shown

Logs
I'm not sure if it's related, but the browser console logs show this before interacting with the page:

XML Parsing Error: syntax error
Location: http://localhost:5988/#/contacts/e3a883de-721f-44ae-8b16-cfe635b18246/add/person
Line Number 1, Column 1: localhost:5988:1:1
Data node: /*/meta/deprecatedID with null-based index: undefined not found. Ignored. inbox.js:206967:20
 action SET_LOADING_CONTENT @ 15:43:02.170 redux-logger.js:1
Translation for contact.type.string.new doesn't exist angular.js:15570
Content Security Policy: The page’s settings blocked the loading of a resource at inline (“script-src”). jquery.js:7531
 action SET_ENKETO_STATUS @ 15:43:02.586 redux-logger.js:1

No further console logs are generated when the secondary datepicker is shown

Screenshots
rando datepicker

Environment

  • Instance: local dev instance checked out from master, currently at 12981f5
  • Browser: Firefox 77.0.1 (64-bit)
  • Client platform: Ubuntu 18.04
  • App: webapp
  • Version: 3.10.0

Additional context

  • The secondary datepicker does show the date I picked in the field, but doesn't update the field if I use it to select a new date
  • The primary datepicker looks to be a non-native HTML element and isn't themed to my desktop. The secondary datepicker looks to be a native HTML datepicker element
  • The issue doesn't manifest in chrome
  • the issue only manifests if you click on the question or-branch non-select invalid-required label for the Age field.
  • when testing on 3.7.1 via an instance on demo-cht.dev.medicmobile.org, I see the same problem when editing an existing person
@mrjones-plip mrjones-plip added the Type: Bug Fix something that isn't working as intended label Jun 11, 2020
@garethbowen garethbowen added the Help wanted Good for first time contributions label Jun 11, 2020
@mrjones-plip mrjones-plip self-assigned this Jun 17, 2020
@mrjones-plip
Copy link
Contributor Author

diving into this and have made very little headway, but I'll get there! In case I get help from others, here's a gif of exactly what I see:
datepicker

@mrjones-plip
Copy link
Contributor Author

mrjones-plip commented Jun 19, 2020

I...I think I found the problem? I think I found the problem!! Our forms are structured to use the implicit relationship of a label tag by virtue of it wrapping around in input tag (as opposed to using the explicit for="ID_HERE" syntax). This is all fine and dandy, except that there's not one, but two inputs inside the label tag in question.

If I go in via the developer tools and delete the first one (which is invisible display: none) then when I click the label the wrong behavior goes away and the correct behavior is seen. w00t! This is the intput tag that's causing the problem in the rendered DOM:

<input type="date" name="/data/contact/ephemeral_dob/dob_calendar" data-required="true()" data-constraint="floor(decimal-date-time(.)) <= floor(decimal-date-time(today()))" data-relevant="not(selected(../dob_method,'approx'))" data-type-xml="date" style="display: none;">

@mrjones-plip
Copy link
Contributor Author

Ah! And I found a possible fix. It appears that the label tag is binding ("binding"?) to the first input inside it. In our problem case, it's this invisible input mentioned above. Re-ordering the DOM so that this input is rendered after the "real" one (though it looks like it's actually the fake one), the problem goes away.

How easy this is to do is another question ;)

Onward!!

@mrjones-plip
Copy link
Contributor Author

mrjones-plip commented Jun 19, 2020

I chatted with @SCdF on this and he said that MM uses the bootstrap datepicker b/c it fixes the way older versions of android poorly handle them with the native picker. He was also able to reproduce the error on his FF instance, which is great 💯

He also said that @garethbowen would likely be the best person to follow up on this ;) I checked your calendar and it looks like you're busy today @garethbowen (Jun 19th) - but when ever you get a sec! Edit - And he also every so gently reminded me that today is Sat for you - catch ya next week!

@garethbowen
Copy link
Member

@mrjones-plip That makes sense to me! Great idea.

Essentially clicking on the label focuses on the hidden input which triggers Firefox to show the datepicker. IMO this is a bug in Firefox because it should not be possible to focus on a hidden input. If you felt so inclined you could raise a bug in Firefox.

It looks like this code in the enketo-core widget is meant to handle it: https://github.com/enketo/enketo-core/blob/4.41.6/src/widget/date/datepicker-extended.js#L143 . This has changed a little in the latest version of enketo-core so it's possible it'll be fixed when we upgrade, but I can't see any issues in the enketo-core repo about this so probably not...

First, double check my assumption that this is the widget that's being used here - we have a few different ones...

If it is then the fix is a little difficult because we'd have to patch the enketo-core library. For now, you can investigate the correct solution by copying the enketo-core widget linked above into our code and updating our widget.js to use the copied version instead of the official version. Make sure you copy the right version of the enketo-core widget as we're stuck on 4.41.6 which is quite an old version.

Then you can use the normal debugging process to test out ways to reorder the DOM, or change the ID/name of elements, or try and catch the focus, or whatever!

Once we've worked out how to solve it we can work out how to get it into the code, either by patching, or extending, or updating enketo-core.

@mrjones-plip
Copy link
Contributor Author

@garethbowen - whew - this is slow going for this PHP engineer over here ;) I've copied over the enketo core (src) into our app and updated widget.js to use that local source per your instructions. I can confirm that when I comment out this line in widget.js:

require( './enketo-core-clone-testing/widget/date/datepicker-extended' ),

The app then devolves as expected to show the native FF datepicker when the dob field gets focus, with no bootstrap UI to be seen.

Now on to (slowly) debug how to fix it!

@mrjones-plip
Copy link
Contributor Author

mrjones-plip commented Jun 23, 2020

Oh - that went a lot more quickly than I thought it would! Whew. All we have to do is change line 94 in datepicker-extended.js from this:

$dateI.hide().after( $fakeDate );

To this:

$dateI.hide().before( $fakeDate );

I'll look into this part now:

Once we've worked out how to solve it we can work out how to get it into the code, either by patching, or extending, or updating enketo-core.

@garethbowen
Copy link
Member

One way to solve it is simply by committing the copied code. We'd just have to be careful to update it (or hopefully if it's fixed, remove it) when updating enketo-core.

@mrjones-plip
Copy link
Contributor Author

Yes, for sure including the copied code wholesale would work, as it is in my dev environment now. However, the datepicker-extended.js file has this at the top:

var Widget = require( '../../js/Widget' );
var support = require( '../../js/support' );
var $ = require( 'jquery' );
var types = require( '../../js/types' );
require( 'bootstrap-datepicker' );
require( '../../js/dropdown.jquery' );

In order to get all those to resolve nicely, I just copied the entire src directory in which is 805k of new JS for just a one line fix. I'm hoping for a fix with a more limited scope. Digging more!

@garethbowen
Copy link
Member

Have a look at our other custom widgets in /webapp/src/js/enketo/widgets/ for inspiration.

@mrjones-plip
Copy link
Contributor Author

Yes, I saw that! More to get confused by ;) I'll pick this up tomorrow - but very much appreciate the hand holding through this!

@mrjones-plip mrjones-plip removed the Help wanted Good for first time contributions label Jun 25, 2020
@mrjones-plip
Copy link
Contributor Author

mrjones-plip commented Jun 30, 2020

@garethbowen - thanks for the tip re FF bug! I just filed a bug with them. An interesting finding is that both Chrome and Safari don't show datepicker but they also do not (visibly?) put the focus on the visible input. For reference the distilled test HTML is:

<label>
A label
<input type="date" style="display: none;" >
<input type="text">
</label>

I wonder if it the other two browsers are indeed putting the focus on the date input but are simply not showing the datepicker? I'll write some more test code to verify - my gut is saying "yes".

A quick test for the form in question in this ticket in chrome reveals that neither datepicker is shown when clicking the the label. Meanwhile all the other labels on this form work as expected. An upstream ticket against enketo is likely warranted too then.

@garethbowen
Copy link
Member

An upstream ticket against enketo is likely warranted too then.

Yes I think so, however we're on a very old version of Enketo so it'd be good to attempt to reproduce this using the latest version before filing a bug. Plunker is a good site for reproducing bugs and means you can share the reproduction if you do end up raising an Enketo bug.

@mrjones-plip
Copy link
Contributor Author

Awesome! As always, your guidance is much appreciated.

@mrjones-plip
Copy link
Contributor Author

OK! I had great luck with how easy it was to reproduce this issue on latest Enketo version using their off the self set up (git clone -> npm install -> grunt -> npm start). Once we see what Enketo says about the the ticket I submitted, along with a one line PR fix, I suspect we can close this ticket and open another "upgrade to latest enketo". Oh, wait, this ticket already exists! w00t! Originally slated for 3.11, but now set for 3.12.

So, cool. When I hear back from Enketo I'll report back and we can decide to close or what.

@mrjones-plip
Copy link
Contributor Author

The Enketo PR was accepted and merged to enketo-core master \o/

closing in favor of the other upgrade cht-core to enketo tickets

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Bug Fix something that isn't working as intended
Projects
None yet
Development

No branches or pull requests

2 participants