fix(select): revert commit to fix skipping over blank disabled options

[ealtenho]

An earlier commit caused an error where the first option of a select would be skipped
over if it had a blank value. This fix adds a test case to identify this case and shows
that reverting the earlier commit causes the behavior to change.

Relates to #7715

[mary-poppins]

Thanks for the PR! Please check the items below to help us merge this faster. See the contributing docs for more information.

• Uses the issue template (#8221)

If you need to make changes to your pull request, you can update the commit with git commit --amend.
Then, update the pull request with git push -f.

Thanks again for your help!

[caitp]

Please use git revert <SHA> and update the commit message to the form revert: fix(select): avoid checking option element selected properties in render (just to be consistent with the style we're using!) You can also change the commit message to include notes about the reason for reverting, that would be helpful.

I'm all for reverting it, so LGTM

[Narretz]

Using git revert is not only for consistency, it also makes it easy to find the actual reverted commit. However, you need a separate commit for the tests in this case.

[caitp]

yes, I think we want to reverts here (or maybe I'm wrong, it's been months!)

[ealtenho]

Thanks @caitp @Narretz, I will move the revert and the test cases into two different commits.

[Narretz]

The commit in question is dc149de, because it caused a lot of different bugs. The consensus was that the original bug should be fixed in FF, is that right @ealtenho ?

[caitp]

different shas in master and v1.2.x

[ealtenho]

Yes @Narretz, I filed a bug with Firefox. @IgorMinar is also looking at how to fix the original issue.

[caitp]

PS @ealtenho do you have a link to the bugzilla bug? I'd like to be CC'd on that

[IgorMinar]

bugzilla link: https://bugzilla.mozilla.org/show_bug.cgi?id=1039047

[ealtenho]

@caitp @IgorMinar how do these look?

Actually, I hadn't updated the comment for the second commit. I think it now has the right message.

[IgorMinar]

@ealtenho

we don't want to revert this:

-    it('should not update selected property of an option element on digest with no change event',
 -        function() {
 -      createSingleSelect();
 -
 -      scope.$apply(function() {
 -        scope.values = [{name: 'A'}, {name: 'B'}, {name: 'C'}];
 -        scope.selected = scope.values[0];
 -      });
 -
 -      var options = element.find('option');
 -      var optionToSelect = options.eq(1);
 -
 -      expect(optionToSelect.text()).toBe('B');
 -
 -      optionToSelect.prop('selected', true);
 -      scope.$digest();
 -
 -      expect(optionToSelect.prop('selected')).toBe(true);
 -      expect(scope.selected).toBe(scope.values[0]);
 -    });
 -

[IgorMinar]

otherwise it looks good

[caitp]

So I was looking at the spec, and it looks like FF is actually totally compliant with the requirements laid out for it (re: http://www.whatwg.org/specs/web-apps/current-work/multipage/the-button-element.html). This is implementation-defined behaviour, so to get it fixed in gecko we really need to make a case for it --- I think we have a case here, but it could very well be WONTFIX'd.

Some mozillians spoken to seem to agree that it's a bug, while some consider it a feature, and the spec just doesn't care one way or the other.

I guess we'll see how it goes

[ealtenho]

@IgorMinar, so it's okay to have a failing test in the PR at this point? I know that the test is still necessary, but I wasn't sure if it should be left if this PR does not contain a fix?

@caitp is there any action I should take then on the bug report? Like elaborating how this behavior affects us?

[caitp]

So, @sideshowbarker seems to agree that it's a weird user experience, and Olli Pettay seems to be at least satisfied that browser interop is an issue which should be addressed.

I've chosen to describe the behaviour as reading a menu in a restaurant and having your credit card charged as soon as you gaze at an item, before actually committing to a decision about your order. Analogies like that might be helpful in explaining why it's a problem, and an explanation as to why we sometimes consider values before the user actually chooses one (eg during digest) would be helpful too.

But frankly, the reproduction is pretty solid and clearly shows the problems with this behaviour, so that should be more than enough.

One thing though, mobile devices (with touch screens) are really a huge priority for pretty much all of the different vendors right now, so it can be hard to explain problems which affect actual pointing devices (this problem won't occur on mobile, since there's no real "hover" scenario on mobile).

[ealtenho]

@IgorMinar I changed the revert to preserve the failing test.
I also added tests for regression #7855

[caitp]

so I guess that bug is a duplicate of a bug opened in 2007 (the search terms I went for didn't include "dropdown menu", so I did't see it before)... that somewhat shrinks my hopes that it will be fixed quickly :( That's a shame.

[sideshowbarker]

so I guess that bug is a duplicate of a bug opened in 2007 (the search terms I went for didn't include "dropdown menu", so I did't see it before)... that somewhat shrinks my hopes that it will be fixed quickly :( That's a shame.

Well that bug is marked as blocking a couple other more recent bugs, so I’d think that increases the chances of a change being made sooner than the 2007 date would suggest. https://bugzilla.mozilla.org/showdependencytree.cgi?id=987143&hide_resolved=1

[jeffbcross]

@ealtenho we should make a separate commit to add the select ngOptions should not update selected property of an option element on digest with no change event test back so that the revert commit is a pure revert. Even so, @IgorMinar must have had something else in mind other than committing a failing test.

[ealtenho]

@jeffbcross I had a discussion with @btford and @IgorMinar about this yesterday and my understanding was that I was submitting this PR partially so @IgorMinar could work on addressing the failing test case.

@IgorMinar

we should make a separate commit to add the select ngOptions should not update selected property of an option element on digest with no change event test

should I implement this?

[petebacondarwin]

we should make a separate commit to add the select ngOptions should not update selected property of an option element on digest with no change event test

@ealtenho - I would definitely do this. It is much easier to understand the failing test if it is not hidden inside some partially reverted commit.

[petebacondarwin]

@IgorMinar - I have assigned this to me for tomorrow. If you want it back just say; if I don't make any progress tomorrow then I'll assign it back to you myself :-)

[petebacondarwin]

Please take a look at my fix for this: #8367

[petebacondarwin]

@ealtenho - LGTM. Let's get a check from one of the core team.

[caitp]

Does this mean that you can conceivably render() multiple times per digest (not including retrying dirtychecking)?

[petebacondarwin]

Well previously render() was being called a minimum of 2 times for every digest, whether or not there was a change to any of the model values.
I initially put these into a watchGroup() call but that doesn't have a parameter to watch deeply.

[caitp]

but we shouldn't really need deep watching afaik? since we only test if values are the same with identity equality anyhow

[caitp]

or is something different now that we do need to do that

[petebacondarwin]

The deep watches are needed because we need to re-render if the list of options changes (valuesFn).
Also, the selectedSet is recreated every time and so we need to check whether its properties have changed.

[caitp]

I'm not really sold on that approach.

Previously, we were doing this work in the watch expression, and knew specifically which options changed and how.

Now, we're deep-watching with the same watch action twice, without knowing what's changed and what hasn't. For small collections of options it won' t matter, so if we're shipping this, really emphasize that it's not intended for large collections or complex collections of options