980075 - Can no longer comment on lines in pull-requests on github.com

Status: RESOLVED FIXED

(Tech Evangelism Graveyard :: English US, defect)

Description

[Paul [:pmac] McLanahan]

Not sure this is a Fx issue or GH, but it works in Release and Chrome.

Something (I've yet to discover what) is causing me to be unable to add comments to lines in various diff views (e.g. pull-requests) on github.com. This is happening in the latest Nightly (30.0a1 (2014-03-05)). I've restarted in safe-mode and tried a fresh profile, and the problem persists.

Expected:

Hovering over a line in a diff on github.com would show an icon to the left side that when clicked would open a comment form for that line.

Actual:

Hovering over a line does not show the icon and gives me no click targets for opening the form.

Example Diff:

https://github.com/mozilla/bedrock/pull/1670/files

The problem is visible whether logged into Github or not.

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Regression range:

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=529b86b92b1d&tochange=e5b09585215f

Comment 2

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Needs bisection.

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

I'm at it. Almost done.

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

The first bad revision is:
changeset:   171670:0199299ac111
user:        Seth Fowler <[e-mail]>
date:        Mon Mar 03 19:40:29 2014 -0800
summary:     Bug 63895 (Part 2) - Support table parts as absolute containing blocks. r=dbaron

Comment 5

[Seth Fowler [:seth] [:s2h]]

Taking this.

Comment 6

[Anthony Ricaud (:rik)]

A lot of Mozilla contributors are running Nightly and using Github (websites, gaia). Can we make sure tomorrow's nightly works again? Either we have a fix or we try to backout the offending patch.

Comment 7

[Seth Fowler [:seth] [:s2h]]

(In reply to Anthony Ricaud (:rik) from comment #6)
> A lot of Mozilla contributors are running Nightly and using Github
> (websites, gaia). Can we make sure tomorrow's nightly works again? Either we
> have a fix or we try to backout the offending patch.

Yes, that sounds like a good plan. I am starting work on this immediately.

Comment 8

[Seth Fowler [:seth] [:s2h]]

So the problem here looks to be that Github has a stray 'position: relative' in their CSS, applied to the table rows on this page. (It comes from class 'file-diff-line'.) With bug 63895 applied, this causes us to *correctly* position the comment icon (which is a 'position: absolute' <b> with class 'add-line-comment') relative to the table row. Since the entire table is inside a div with class 'highlight' that sets 'overflow: auto', we clip the comment icon and it's not visible.

This works in browsers that ignore the 'position: relative' on the table row, because they position the comment icon relative to the div with class 'file', which has 'position: relative' set and is *outside* the 'highlight' div with 'overflow: auto' set. This means they don't apply the 'overflow: auto' to the comment icon and so they don't clip it.

Here's a JSFiddle with a test case demonstrating the problem:

http://jsfiddle.net/Mu7QN/12/

On browsers that support 'position: relative' on table rows, you'll see one 'X' on the left. On browsers that don't support it, you'll see two 'X's on the left.

Comment 9

[Seth Fowler [:seth] [:s2h]]

Attachment: Testcase: Table row as absolute containing block.

Better yet, here's the same test as an attachment to the bug.

Comment 10

[Seth Fowler [:seth] [:s2h]]

Just checked Chrome, Safari, and IE 5.5 - 11. None of them support treating table rows as absolute containing blocks. So it's not a surprise that Github hasn't noticed this problem before - we appear to be the first browser to support this!

Comment 11

[Seth Fowler [:seth] [:s2h]]

Alright, I've added some diagnostic information to identify pages that apply 'position: relative' to table parts other than cells, and surfed around a fairly large selection of popular websites. I may well have just gotten lucky (I'm not in any way claiming this is a rigorous study or a large sample) but I didn't trigger the warning once other than on this Github page.

For now I'm going to proceed under the assumption that pages that trigger this are quite rare in the wild. I'm going to back out this patch, let Github know about the problem and give them a chance to fix it, and then reland.

If it turns out that *lots* of pages have this problem, we can reevaluate. I'd like to keep this functionality if it's feasible to do so, though.

Comment 12

[Seth Fowler [:seth] [:s2h]]

Note that the patch in bug 63895 has now been backed out and Github has been notified. We'll continue tracking this issue in this bug, though.

Comment 13

[Paul [:pmac] McLanahan]

Fantastic work :seth! Sounds like a great way forward. Thanks!

Comment 14

[Anthony Ricaud (:rik)]

I still have the issue on today's nightly. I'm on b0e7f63c2138. Is that expected? (I have no HG skills to check that myself)

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Correct, the backout missed today's nightly.

Comment 16

[Seth Fowler [:seth] [:s2h]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #15)
> Correct, the backout missed today's nightly.

:sadface:

Sorry about that, folks! Will be resolved tomorrow, then.

Comment 17

[Seth Fowler [:seth] [:s2h]]

Github has been totally awesome about this; they got their CSS updated to fix this problem within 24 hours. Massive internet points to them. Because of that, I've marked this as resolved, and have just relanded bug 63895.