1326163 - Unable to interact with text fields / text at top of SurveyMonkey "State of Code Review Survey"

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, defect)

Description

[Daniel Holbert [:dholbert]]

Filing this for https://github.com/webcompat/web-bugs/issues/4095

STR:
 1. Visit https://www.surveymonkey.com/r/2016SOCR?mkt_tok=eyJpIjoiTjJSaVptUTVPV1U0TXpKbSIsInQiOiJGdm16ODZxRHJcL1BMZWZYRFwvUFhxeHEzTDdmK1dmelhoUTVVdStqU3NISGZVQUpQSWd4ODRpRnltdW44WGc5ckJSWWRYeW42TGJDZzFLQ2lldTdcLzV6eTJpbERpQzBkN0YzaDJsbnZYYXVnakd1WG9cL1wvbU5QRDRtVDRubnh0bE9QIn0=

 2. Try to click one of the first few text fields (and enter text). Or try to select the "About me" title text above the first text field.

ACTUAL RESULTS:
Clicks have no effect. "Inspect" shows that they're blocked by several large transparent <fieldset> elements.  (Though if you set all of these <fieldset> elements to display:none, then the clicks finally get through.)

EXPECTED RESULTS:
Clicks should have an effect -- focusing the textfield, selecting text, etc.


Firefox release (50), Chrome 55 and Edge 14 give EXPECTED RESULTS.
Firefox 53 Nightly gives ACTUAL RESULTS. So, this is a regression. I suspect bug 504622, based on preliminary mozregression results.

Comment 1

[Daniel Holbert [:dholbert]]

Yup, mozregression gives this range:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=f4a2a329a7080cab9b7ebd1eb4b3295a1232b775&tochange=b55a8d9517c8a4cace18cd153e84ed0a0f0c5653
...which (based on the <fieldset> being involved) points the blame at bug 504622.

Comment 2

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #0)
> Clicks have no effect. "Inspect" shows that they're blocked by several large
> transparent <fieldset> elements.  (Though if you set all of these <fieldset>
> elements to display:none, then the clicks finally get through.)


Actually, the <fieldset>s aren't so big -- but their *overflow area* is absurdly huge (running off the top of the page).

You can see this if you select the topmost one (via Inspect Element) and then add both "border" and "outline" to it, and compare them.

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: screencast showing the topmost <fieldset> in question and its huge overflow area

Comment 4

[Boris Zbarsky [:bzbarsky]]

The overflow area looks just as big for me in Firefox 50.

I expect the problem is nsDisplayFieldSetBorderBackground::GetBounds.  It includes the whole overflow area, but for hit-testing this is certainly going to be wrong.  Chances are, we should just do the complicated thing there instead.  :(

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: Don't claim a too-big bounds for a fieldset's border/background display item, because that will confuse hit testing

Comment 6

[Karl Dubost💡 :karlcow]

This is also breaking politico.com
https://webcompat.com/issues/4092

Comment 7

[Karl Dubost💡 :karlcow]

Breaking http://www.lemonde.fr/
1. Go to http://www.lemonde.fr/
2. Scroll down to "Annonces Auto", switch to "Annonces Immo"
3. Try to click on the thumbnails, the click is blocked.

Visible with putting outline and border. 

So basically any major Web site which has a fieldset is currently possibly broken.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: Now with a test

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Instead, maybe this display item should override HitTest the same way that nsDisplayBackgroundColor::HitTest and nsDisplayBackgroundImage::HitTest do, instead of just unconditionally appending in HitTest?  That seems more consistent with the way other things work.

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(This is different in two ways:  on other elements, border-image overhang doesn't hit-test, and also areas clipped by rounded corners don't hit-test.)

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8822541 [details] [diff] [review]
Now with a test

see comment 9 and comment 10

Comment 12

[Boris Zbarsky [:bzbarsky]]

Hmm.  Yes, that might make more sense.

I just checked, and rounded corners hit-test in Fx50 for fieldsets, so that's a preexisting bug.

I tried doing the HitTest thing, and it does fix this bug, but clicking outside a rounded border still hits the fieldset.  I'm not sure why so far, because we _are_ now taking the relevant early return in HitTest.  Looking into it.

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Actually, it's not clear to me that this item needs a HitTest method at all.  https://hg.mozilla.org/mozilla-central/rev/5453b1ce4e85e98d358c12c8dc168a9f484b7491 changed the display item so it's only responsible for painting border, and fieldsets have normal background items.  (The item should also be renamed to nsDisplayFieldSetBorder... and really should have been there, in bug 1227327.)

Comment 14

[Boris Zbarsky [:bzbarsky]]

OK, so the reason clicking outside the rounded border still hits the fieldset is that it hits the display items for the anonymous frame.

Comment 13 makes sense to me, I think.  Will post an updated patch in a minute.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. The fieldset's border drawing display item shouldn't participate in hit-testing. That's handled by its background item already

Also, fix the fieldset's inner frame to not leak out of it when it has rounded
borders and mess up hit-testing in the corner areas.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Rename nsDisplayFieldSetBorderBackground to nsDisplayFieldSetBorder, since that's what it does

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8822564 [details] [diff] [review]
part 2.  Rename nsDisplayFieldSetBorderBackground to nsDisplayFieldSetBorder, since that's what it does

The commit message should reference bug 1227327 and perhaps changeset 5453b1ce4e85e98d358c12c8dc168a9f484b7491, since that's what made it do so.

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

But maybe put the border-radius change in a separate changeset?  (A test would be nice, too, but probably not necessary...)

Comment 19

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 1 without the border-radius change

Comment 20

[Boris Zbarsky [:bzbarsky]]

Attachment: Just the border-radius bit, with test

Comment 21

[Boris Zbarsky [:bzbarsky]]

Attachment: Class rename with the better commit message

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6d65a2f7bf2d
part 1.  The fieldset's border drawing display item shouldn't participate in hit-testing.  That's handled by its background item already. r=dbaron
https://hg.mozilla.org/integration/mozilla-inbound/rev/9b51d74e8f3f
part 2.  Inherit a fieldset's border-radius down into its content wrapper box, so it doesn't leak out and mess up hit-testing in the corner areas when it has rounded borders.  r=dbaron
https://hg.mozilla.org/integration/mozilla-inbound/rev/619aabf58ead
part 3.  Rename nsDisplayFieldSetBorderBackground to nsDisplayFieldSetBorder, since that's what it does ever since bug 1227327 was fixed in <https://hg.mozilla.org/mozilla-central/rev/5453b1ce4e85e98d358c12c8dc168a9f484b7491>. r=dbaron

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6d65a2f7bf2d
https://hg.mozilla.org/mozilla-central/rev/9b51d74e8f3f
https://hg.mozilla.org/mozilla-central/rev/619aabf58ead

Comment 24

[Karl Dubost💡 :karlcow]

politico.com fixed
surveymonkey.com fixed
www.lemonde.fr fixed

Thanks a lot. Everyone (and specifically Daniel, Boris, David). For the very quick fix. 
Happy new 2017 webcompat year.

Comment 25

[Henrik Skupin [:whimboo][⌚️UTC+1]]

It looks like that this patch also fixed the non-clickable links in the sidebar for a German banking site (dkb.de), which has been reported as a problem to me end of December.

Comment 26

[Simon Pieters [:zcorpan]]

Test for this in https://github.com/web-platform-tests/wpt/pull/12616