Closed Bug 1178722 Opened 9 years ago Closed 8 years ago

Select with multiple selection - options visibility not respected in popup

Categories

(Firefox for Android Graveyard :: General, defect)

38 Branch
ARM
Android
defect
Not set
normal

Tracking

(firefox48 fixed, fennec+)

RESOLVED FIXED
Firefox 48
Tracking Status
firefox48 --- fixed
fennec + ---

People

(Reporter: nicolae_albu2004, Assigned: tldmartin, Mentored)

References

Details

(Whiteboard: [lang=js][good next bug])

Attachments

(4 files, 5 obsolete files)

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:38.0) Gecko/20100101 Firefox/38.0
Build ID: 20150525141253

Steps to reproduce:

1. Open a page with a a <select> with multiple selection that has options hidden (for example with display: none).
1.1. I attached a simple test case. The even options should be hidden.
2. Observe that the page render respects the visibility.
2.1. On my test case, the even options are hidden (good).
3. Tap on the field to bring the selection popup.
4. The popup opens.


Actual results:

The popup displays all options, regardless if they're set to be hidden. 


Expected results:

The popup should have displayed only the visible ones, like on the page render.
OS: Unspecified → Android
Hardware: Unspecified → ARM
Here is a screenshot that compares the options on the page with the ones on the popup.
The elements that do not appear in desktop are `display: none`.

Finkle, do you know off-hand where we can handle this code?
Flags: needinfo?(mark.finkle)
Any news?
Nicolae, if Finkle does not get back sooner, this will be seen in our triage a week from today where it will be prioritized and potentially assigned.
tracking-fennec: --- → ?
Status: UNCONFIRMED → NEW
tracking-fennec: ? → +
Ever confirmed: true
Flags: needinfo?(mark.finkle) → needinfo?(margaret.leibovic)
The logic you want to change is here:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/SelectHelper.js#75
Mentor: margaret.leibovic, michael.l.comella
Flags: needinfo?(margaret.leibovic)
Whiteboard: [lang=js][good next bug]
(In reply to :Margaret Leibovic from comment #5)
> The logic you want to change is here:
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/
> SelectHelper.js#75

Hi,

Are you suggesting we should add a test like |if(aNode.style.display!=="none")| inside |getListForElement| so that it will not return any hidden options?

I tried doing that and it successfully stopped hidden options from appearing in the prompt. Unfortunately it also caused a new problem: the options that you click on will not always be the options that get selected after clicking "Done". This appears to be because the Java code (see links below) assumes that option IDs and indexes will match, which won't be the case if we remove the hidden options.

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/prompts/Prompt.java#428
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/prompts/PromptListAdapter.java#99

If this is correct, I'm sure I could hack together a fix but I'm not sure what approach is most consistent with Fennec's architecture. What are your thoughts?

Thank you :)
Tristan
Flags: needinfo?(margaret.leibovic)
Sorry for the slow response, great work investigating this!

It's unfortunate that excluding the hidden elements doesn't just work. It's been a while since I've dug into this code - would it be possible to handle this information on the JS side when we get the selected items back from Java?

Or alternately, could we mark the item as hidden when we pass the list to Java, then handle hiding the item in Java?

It sounds like you're on the right track, I trust you can come up with a reasonable solution. Feel free to post a patch early for feedback, and I'll do my best to respond more quickly.
Assignee: nobody → tldmartin
Flags: needinfo?(margaret.leibovic)
Attached patch 1178722.patchSplinter Review
This should do the trick :^)
Flags: needinfo?(margaret.leibovic)
For my own education, would it have been possible to set an attribute on the JS object that would match up against a UI element defined in XML/XUL and hide the list item that way?
Flags: needinfo?(margaret.leibovic)
Oops sorry - learning how to use bugzilla :P
Flags: needinfo?(margaret.leibovic)
Comment on attachment 8727218 [details] [diff] [review]
1178722.patch

Review of attachment 8727218 [details] [diff] [review]:
-----------------------------------------------------------------

When uploading a patch, you should set the review? flag to request review. I'll take a look at this today!
Attachment #8727218 - Flags: review?(margaret.leibovic)
Comment on attachment 8727218 [details] [diff] [review]
1178722.patch

Review of attachment 8727218 [details] [diff] [review]:
-----------------------------------------------------------------

Nice work! I tested this with the included testcase, and it works as expected. I'd be fine for us to land this as-is, but it would be great to file a follow-up to add some automated tests for this code, since it's pretty tricky.

::: mobile/android/chrome/content/SelectHelper.js
@@ +56,5 @@
>              aNode.selected = true;
>            }
> +          if (SelectHelper._isListItemVisible(aNode)) {
> +            i++;
> +          }

This seems tricky and fragile, but a better approach isn't immediately clear to me. Could you add some comments in here explaining why we need to be doing this?

Also, how many testcases did you use to test this? I'm worried that we don't have automated tests for this, and I want to make sure we're not introducing any regressions.

Alternately, would you be interested in writing an automated test? It might be tricky because we're trying to test something that crosses the JS/Java divide. If you want to write a test purely in JS, chrome mochitests are a good option:
https://wiki.mozilla.org/Mobile/Fennec/Android/Testing#mochitest_.28plain_and_chrome.29

Existing tests live here:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/tests/browser/chrome/
Attachment #8727218 - Flags: review?(margaret.leibovic) → review+
Flags: needinfo?(margaret.leibovic)
Keywords: checkin-needed
Thanks Margaret. I'll add some //comments and give writing the tests a shot.
https://hg.mozilla.org/mozilla-central/rev/365bcf3730e8
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Attached patch 1244197comments.patch (obsolete) — Splinter Review
> This seems tricky and fragile, but a better approach isn't immediately clear
> to me. Could you add some comments in here explaining why we need to be
> doing this?

It seems like the previous patch has already been merged in, so I created a new patch for comments. I'm not sure if that's the correct approach.

> Also, how many testcases did you use to test this? I'm worried that we don't
> have automated tests for this, and I want to make sure we're not introducing
> any regressions.

I tested it with the HTML page Nicolae attached. I also modified it to ensure it wasn't case sensitive to styling (ie, "display: nOnE") and that hiding via CSS behaved the same.

> Alternately, would you be interested in writing an automated test? It might
> be tricky because we're trying to test something that crosses the JS/Java
> divide. If you want to write a test purely in JS, chrome mochitests are a
> good option:

I attempted this, but it seems to require a lot of knowledge of XPCOM and other things that are new to me. I can revisit this when I know more, but we both know how dependable vague intentions like that are...
Flags: needinfo?(margaret.leibovic)
Attached patch 1178722comments.patch (obsolete) — Splinter Review
Attachment #8730049 - Attachment is obsolete: true
(In reply to Tristan from comment #16)
> Created attachment 8730049 [details] [diff] [review]
> 1244197comments.patch
> 
> > This seems tricky and fragile, but a better approach isn't immediately clear
> > to me. Could you add some comments in here explaining why we need to be
> > doing this?
> 
> It seems like the previous patch has already been merged in, so I created a
> new patch for comments. I'm not sure if that's the correct approach.

Thanks! I must have jumped the gun marking this as checkin-needed. I can check in this follow-up patch for you myself.
 
> > Also, how many testcases did you use to test this? I'm worried that we don't
> > have automated tests for this, and I want to make sure we're not introducing
> > any regressions.
> 
> I tested it with the HTML page Nicolae attached. I also modified it to
> ensure it wasn't case sensitive to styling (ie, "display: nOnE") and that
> hiding via CSS behaved the same.

Thanks for confirming this testing.

> > Alternately, would you be interested in writing an automated test? It might
> > be tricky because we're trying to test something that crosses the JS/Java
> > divide. If you want to write a test purely in JS, chrome mochitests are a
> > good option:
> 
> I attempted this, but it seems to require a lot of knowledge of XPCOM and
> other things that are new to me. I can revisit this when I know more, but we
> both know how dependable vague intentions like that are...

I agree the test framework isn't very approachable, but you should be able to get something basic up and running by copy/pasting from existing tests. Can you file a follow-up bug to track this work? You don't need to solve it immediately, but having a bug on file will be good for remembering that this issue exists.
Flags: needinfo?(margaret.leibovic)
Attachment #8730050 - Flags: review+
See Also: → 1256570
Started new bug report to add a regression test https://bugzilla.mozilla.org/show_bug.cgi?id=1256570
Attached patch 1178722-C.patch (obsolete) — Splinter Review
Hi Margaret. I've got some mochitests working. I also discovered that my patch wasn't great because:
1) It did not work for the HTML5 "hidden" attribute. In this updated patch, we test the "computed style" for the element.
2) Although it selected the option elements you chose on the prompt, it would also sometimes select additional hidden elements. *facepalm*

Since I'm also fixing the fix, I thought it best to upload the patch here and simply close the bug report for missing tests. 

Btw, I think this fix is also neater than my previous one. I simply changed forOptions(..) into forVisibleOptions(..). I could not find anywhere else in the codebase this method was being used.
Flags: needinfo?(margaret.leibovic)
(In reply to Tristan from comment #20)

> Since I'm also fixing the fix, I thought it best to upload the patch here
> and simply close the bug report for missing tests. 

Sure, that's fine, but in general, we like to land follow-up fixes in new bugs so that it's easier to keep track of where things have landed. Since the patch here landed in Firefox 48, and that's still mozilla-central, all of these patches will end up in the 48 release, so it won't pose as much of a problem.
Status: RESOLVED → REOPENED
Flags: needinfo?(margaret.leibovic)
Resolution: FIXED → ---
Comment on attachment 8727218 [details] [diff] [review]
1178722.patch

Marking this bug as checked in. I still forgot to land your comment fixes for you, but we can do that when we land the follow-up patch.
Attachment #8727218 - Flags: checkin+
Do you have level 1 commit access? If not, you should get it so that you can push your patches to try server. There are instructions here:
https://www.mozilla.org/en-US/about/governance/policies/commit/
https://wiki.mozilla.org/Try

Also, are you looking for me to review this patch? If so, you should set the review? flag on the patch to request review from me :)
Flags: needinfo?(tldmartin)
Flags: needinfo?(tldmartin) → needinfo?(margaret.leibovic)
Attachment #8732733 - Flags: review+
Comment on attachment 8732733 [details] [diff] [review]
1178722-C.patch

Sorry, I wasn't clear in my last comment. This is how you request review on a patch (no need for the needinfo? flag).
Flags: needinfo?(margaret.leibovic)
Attachment #8732733 - Flags: review+ → review?(margaret.leibovic)
Comment on attachment 8732733 [details] [diff] [review]
1178722-C.patch

Review of attachment 8732733 [details] [diff] [review]:
-----------------------------------------------------------------

This is so great, thanks! As I mentioned in a previous comment, you should get level 1 commit access so that you can push this to try server to verify that it passes consistently on automation. Once that's verified, we can go ahead and land this.

::: mobile/android/chrome/content/SelectHelper.js
@@ +23,5 @@
>  
> +  // This is a callback function to be provided to prompt.show(callBack).
> +  // It will update which Option elements in a Select have been selected
> +  // or unselected and fire the onChange event.
> +  _promptCallBack: function(data, aElement) {

Nit: While you're changing things here, get rid of the "a" prefix on this parameter.

@@ +46,5 @@
> +        i++;
> +      });
> +
> +      if (changed)
> +        this.fireOnChange(aElement);

Nit: While you're here, add braces to this single-line if statement.

@@ +67,5 @@
>      }
> +    let callBack = function(data) {
> +      SelectHelper._promptCallBack(data,aElement)
> +    };
> +    p.show(callBack.bind(this));

Instead of using `bind`, you can use fat arrow syntax to maintain the same context for the function. I believe something like this would do what you want:

p.show((data) => {
  this._promptCallback(data, aElement);
});
Attachment #8732733 - Flags: review?(margaret.leibovic) → review+
I'm away for the next 5 days but will do :)
Attached patch 1178722-C.patch (obsolete) — Splinter Review
Here's the updated patch. I also made the rest of the file consistent with those style corrections.
Attachment #8732733 - Attachment is obsolete: true
Attachment #8737039 - Flags: review?(margaret.leibovic)
Comment on attachment 8737039 [details] [diff] [review]
1178722-C.patch

Review of attachment 8737039 [details] [diff] [review]:
-----------------------------------------------------------------

Nice, thanks. Do you have try server access yet? If not, I can push this to try for you (but it would be good for you to learn how to do it yourself :).
Attachment #8737039 - Flags: review?(margaret.leibovic) → review+
Not yet. Apparently there's a change freeze and I'll get access after that. 

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

Thanks :)
These last two patches that haven't landed don't apply for me locally. Can you update your tree and try re-exporting the patches?

If I can apply them locally, I'll make a try push for you.
Flags: needinfo?(tldmartin)
Attached patch patch for 1178722 (obsolete) — Splinter Review
Attachment #8730050 - Attachment is obsolete: true
Attachment #8737039 - Attachment is obsolete: true
Flags: needinfo?(tldmartin)
Attachment #8740277 - Flags: review?(margaret.leibovic)
Comment on attachment 8740277 [details] [diff] [review]
patch for 1178722

Review of attachment 8740277 [details] [diff] [review]:
-----------------------------------------------------------------

In the future, it would be better to break a patch like this into two commits: one to fix style issues, followed by another to fix the logic. When they are combined it's hard to review the logic changes.

Also, I only realized this after reviewing this that you were just rebasing this patch! As far as I can tell, nothing changed since your previously r+ed patch, right?

::: mobile/android/chrome/content/SelectHelper.js
@@ +73,5 @@
>    },
>  
> +  _isMenu: function(element) {
> +    return (element instanceof HTMLSelectElement ||
> +    element instanceof Ci.nsIDOMXULMenuListElement);

Nit: Put this all on a single line.
Attachment #8740277 - Flags: review?(margaret.leibovic) → review+
(In reply to :Margaret Leibovic from comment #35)
> In the future, it would be better to break a patch like this into two
> commits: one to fix style issues, followed by another to fix the logic. When
> they are combined it's hard to review the logic changes.

Thanks, that seems like good advice for other projects too.

> Also, I only realized this after reviewing this that you were just rebasing
> this patch! As far as I can tell, nothing changed since your previously r+ed
> patch, right?

Yup, that's right.

> 
> ::: mobile/android/chrome/content/SelectHelper.js
> @@ +73,5 @@
> >    },
> >  
> > +  _isMenu: function(element) {
> > +    return (element instanceof HTMLSelectElement ||
> > +    element instanceof Ci.nsIDOMXULMenuListElement);
> 
> Nit: Put this all on a single line.

I updated the patch to fix this nit. It was the only change I made. 

By the way, do you know how difficult it is to use Git on Mozilla projects? It seems doable (eg, https://github.com/mozilla/moz-git-tools ). It's what I'm familiar with and I'd prefer to deepen my knowledge with Git than broaden it with another tool.
Attachment #8740277 - Attachment is obsolete: true
Attachment #8741529 - Flags: review?(margaret.leibovic)
Attachment #8741529 - Flags: review?(margaret.leibovic) → review+
(In reply to Tristan from comment #36)

> By the way, do you know how difficult it is to use Git on Mozilla projects?
> It seems doable (eg, https://github.com/mozilla/moz-git-tools ). It's what
> I'm familiar with and I'd prefer to deepen my knowledge with Git than
> broaden it with another tool.

I don't think it's hard. I don't use it myself, so I'm not the best person to ask, but people definitely do use git. You could ask for advice in #mobile on irc.mozilla.org.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/6f8f469a45c0
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.