1226293 - Greyhound website won't search schedules

Status: VERIFIED FIXED

(Core :: General, defect)

Description

[Josh Matthews [:jdm]]

STR:
1. Visit https://greyhound.ca/default.aspx
2. Choose "Toronto, ON" for Leaving From, and "Guelph, ON" for Going To
3. Press "Search schedules"

Expected:
Overlay explaining that a search is in progress, followed by navigating to search results.

Actual:
JS error: "TypeError: $find(...).get_selectedItem(...) is null"

I can reproduce this in the 11/18 nightly but not Developer Edition or Chrome.

Comment 1

[Boris Zbarsky [:bzbarsky]]

So the basic problem is that on this RadComboBox object get_value returns "".  Then findItemByValue returns null because "" is falsy, etc.

get_value() just returns this._value, so something went wrong earlier...  Really need that regression range.  Josh, are you working on that?

Comment 2

[Josh Matthews [:jdm]]

Yes.

Comment 3

[Josh Matthews [:jdm]]

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=798c82c8a96d1099cf712434d9c9036fc48d8297&tochange=9160f08660b8290559e427fd80d617edd86fe2a6

My guess of the HTMLElement.innerText change was right on the money.

Comment 4

[Boris Zbarsky [:bzbarsky]]

[Tracking Requested - why for this release]: Website bustage.

Comment 5

[Mike Taylor [:miketaylr]]

I think we're bombing out somewhere related to the "Discount Type:" select (Telerik RadComboBox control widget thingy). If you pick something no value is displayed.

$find('ctl00_body_search_discounts').get_selectedItem()._text in "Companion Fare" in Chrome, but "" in Nightly.

Comment 6

[Mike Taylor [:miketaylr]]

(uhhh, it seems like someone is working on this site... this line was commented out between refreshes and now the search works for me (w/ some additional console errors):

//    var discount = $find(SearchReferences.ListDiscounts).get_value();

Comment 7

[Boris Zbarsky [:bzbarsky]]

Ugh. We should have saved as "web page, complete" up front.  :(

Comment 8

[Mike Taylor [:miketaylr]]

Now it's back to not working. Sorry for the noise. Looking into how the RadComboBox expects to get its _text property.

Comment 9

[Kohei Yoshino [:kohei]]

Apparently Firefox returns "" for innerText if the element is hidden. Testcase coming.

Comment 10

[Mike Taylor [:miketaylr]]

(In reply to Kohei Yoshino [:kohei] from comment #9)
> Apparently Firefox returns "" for innerText if the element is hidden.
> Testcase coming.

If that's the case, then that explains RadComboBoxItem.prototype.get_text returning "":

get_text: function() {
    if (this._text !== null) {
        return this._removeEmTags(this._text);
    }
    if ((this._text = this._properties.getValue('text', null)) != null) {
        return this._removeEmTags(this._text);
    }
    if (!this.get_element()) {
        return '';
    }
    var c = this.get_textElement();
    if (!c) {
        return '';
    }
    if (typeof(c.innerText) != 'undefined') {
        this._text = c.innerText;
    } else {
        this._text = c.textContent;
    }
    if ($telerik.isSafari2) {
        this._text = c.innerHTML;
    }
    return this._removeEmTags(this._text);
},

Comment 11

[Kohei Yoshino [:kohei]]

Attachment: testcase

The console should say "Mozilla Firefox developers" but the actual output is "Mozilla Firefox".

Comment 12

[Boris Zbarsky [:bzbarsky]]

Yeah, so GetInnerText will fall back on GetTextContent in the no-frame case but only if we have no style context or are styled display:none or are not in the document.

Should we just always fall back?

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Oops, yes! The spec and the implementation (and the tests) should check whether we're in a display:none subtree instead of just display:none.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Does it have to be a display:none subtree?  For example, could it be some random block inside SVG but not inside foreignObject, which doesn't get rendered as a result?

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Boris Zbarsky [:bz] from comment #14)
> Does it have to be a display:none subtree?  For example, could it be some
> random block inside SVG but not inside foreignObject, which doesn't get
> rendered as a result?

As discussed on IRC, that would be hard to spec properly.

I've updated the spec to say that we fall back to textContent if the element *or any ancestor* is display:none and I'm about to attach my patch.

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: MozReview Request: Bug 1226293. innerText for non-display:none elements with display:none ancestors should use textContent. r=bz

Bug 1226293. innerText for non-display:none elements with display:none ancestors should use textContent. r=bz

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8689902 [details]
MozReview Request: Bug 1226293. innerText for non-display:none elements with display:none ancestors should use textContent. r=bz

https://reviewboard.mozilla.org/r/25743/#review23219

::: dom/html/nsGenericHTMLElement.cpp:3337
(Diff revision 1)
> -      nsComputedDOMStyle::GetStyleContextForElementNoFlush(this, nullptr, nullptr);
> +    if (!presShell || !IsInComposedDoc() ||

if !IsInComposedDoc(), then nsComputedDOMStyle::GetPresShellForContent will return null, no?  So I don't think you need the check.

Speccing the !presShell condition will be hard, by the way.  Not much we can do about that, though.

::: testing/web-platform/tests/innerText/getter-tests.js:76
(Diff revision 1)
>  

It would be good to have tests for the <div> in <svg> case, both with and without display:none on the div (and possibly with an extra level of nesting, with the display:none on the inner thing, too, as a separate test), just to ensure that part works correctly.

r=me

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=35de63c7ef87

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ef653e3a6b0d21190af39dab7cc0716974dbf0f8
Bug 1226293. innerText for non-display:none elements with display:none ancestors should use textContent. r=bz

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a97d937370729c70bb4e55893e05aec556568e45
Bug 1226293. Followup: add SVG tests. r=bz

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ef653e3a6b0d
https://hg.mozilla.org/mozilla-central/rev/a97d93737072

Comment 22

[u279076]

Verified fixed in today's Nightly.