407817 - Fast find (find bar) finds text hidden in the overflow of "overflow:hidden" elements but does not show it (scroll to it)

Status: NEW

(Toolkit :: Find Toolbar, defect, P5)

Description

[Ryan Jones-Ward [:sciguyryan]]

Recently I noticed this but I guess it has always been this way.

Reading E-mails with Google Mail I was searching for text (if you have a Bugzilla E-mail and its description is larger than the page width you can see this yourself by searching for "comments to this bug at"). Sometimes it finds content that is completely invisible to the user, I believe its overflowed in this case, and this could potentially cause some confusion as the find bar is claiming to have found something but nothing is shown.

I'm not sure what can actually be done here, maybe set the find bar an intermediate between the not found and found for when the content isn't actually visible too the user?

Comment 1

[:Gavin Sharp [email: [e-mail]]]

Bug 305381?

Comment 2

[Ryan Jones-Ward [:sciguyryan]]

I'm not sure that its the same bug because that bug doesn't seem to mention overflow which is what I believe is happening here.

Comment 3

[:Gavin Sharp [email: [e-mail]]]

Depends on to track might be a good idea, then.

Comment 4

[Martijn Wargers (dead)]

Attachment: testcase

I don't think this would be fixed by bug 407817.
Apparently, overflow: hidden can't be scrolled anymore within the find code. This used to be possible in Mozilla1.7. I think dbaron disabled it in some bug.

It seems to me that Mozilla should be able to scroll the found word into view for overflow: hidden nodes. That's also possible in IE. (Opera and Safari both show the same behavior as current Mozilla).

Comment 5

[Martijn Wargers (dead)]

(In reply to comment #4)
>  I think dbaron disabled it in some bug.

Ah, found it, bug 259615.
So this is probably a regression from that bug, at least, if the desired behavior is to scroll the found word into view (instead of not finding anything).

Comment 6

[:Gavin Sharp [email: [e-mail]]]

Ah, indeed.

Comment 8

[Adam]

Attachment: Passes SCROLL_OVERFLOW_HIDDEN along in flags so that scrolling occurs; cleans up enumerations/flags to avoid conflict.

The ability to scroll when overflow: hidden made it in since this bug was last visited, it just needed to be connected.

Not sure the best way to test this, or where a test should be added since typeaheadfind doesn't seem to have its own tests?  

The SCROLL_OVERFLOW_HIDDEN flag gets added to the nsISelectionController and nsSelection.cpp to allow translating to the nsIPresShell::SCROLL_OVERFLOW_HIDDEN (simply reusing it would have caused a conflict between that and nsISelectionController::SCROLL_FIRST_ANCESTOR_ONLY, which are both 0x02).  

The changes in nsPresShell.cpp are to avoid ambiguity between nsISelectionController::SCROLL_OVERFLOW_HIDDEN and nsIPresShell:SCROLL_OVERFLOW_HIDDEN.  A few extraneous changes there were line-ending changes.

Comment 9

[:aceman]

Will you request a review or is it not finished yet?

Comment 11

[Dietrich Ayala (:dietrich)]

Comment on attachment 566620 [details] [diff] [review]
Passes SCROLL_OVERFLOW_HIDDEN along in flags so that scrolling occurs; cleans up enumerations/flags to avoid conflict.

I'm not the right reviewer here, haven't ever looked at any of this code.

Can someone suggest reviewers for the respective content, layout and browser changes in this patch?

Comment 12

[:aceman]

https://wiki.mozilla.org/Modules/Toolkit
It seems Gavin (comment 6) is in the list for "Find toolbar".

Comment 13

[Dietrich Ayala (:dietrich)]

Assigned reviewers from the 3 different areas of code in the patch.

Comment 14

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 566620 [details] [diff] [review]
Passes SCROLL_OVERFLOW_HIDDEN along in flags so that scrolling occurs; cleans up enumerations/flags to avoid conflict.

The toolkit/ change is trivially correct, assuming the rest of the patch is OK. r=me on that.

Comment 15

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

I really think the browser should *never* scroll elements that are overflow:hidden; that should be done only by the page, if it wants to.  We have some bugs where sometimes user actions can cause such elements to scroll, but I consider those to be bugs.

Comment 16

[Adam]

Attachment: Alternate patch: don't find hidden text in overflow:hidden areas.

I mostly agree about not scrolling areas that the user can't scroll themselves, so here is an alternative patch that will prevent finding it and confusing them.

Basically:
1. Get the rectangle for the range.
2. Escalate up the chain of frames to find the top frame that is a parent of one with overflows due to styling.  If none is found, then it's visible.
3. Check if the visible rectangle of that frame intersects the range's rectangle.
4. If not, then it's not visible.

"Select all" will still select the hidden text, so it can be copied.  Not sure if there's a bug filed for that (a brief search didn't turn one up).

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 566620 [details] [diff] [review]
Passes SCROLL_OVERFLOW_HIDDEN along in flags so that scrolling occurs; cleans up enumerations/flags to avoid conflict.

I agree with dbaron. overflow:hidden should not be scrollable.

Though, this bug should get some input from ux.

Comment 18

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

Comment on attachment 570125 [details] [diff] [review]
Alternate patch: don't find hidden text in overflow:hidden areas.

While this seems like perhaps a reasonable approach (though I'm actually pretty skeptical that it's worth the code complexity needed to implement it), this patch doesn't really do it right -- it's more complicated than this.  For example, if you have an overflow:scroll div inside an overflow:hidden div, and there's text that you need to scroll the overflow:scroll div in order to show, this will determine that it's hidden because the overflow:hidden div clips it at the overflow:scroll div's *current* scroll position.  And, looking more closely, I think there are a number of other serious bugs in the logic -- for example, considering only the outermost scroll frame that has overflow and not any inside it.

That said, I tend to think this bug should be WONTFIX rather than trying to improve this patch.

Comment 19

[sideshowbarker [:sideshowbarker]]

If you try the https://bugzilla.mozilla.org/attachment.cgi?id=292660 case in IE, Chrome, Opera, and WebKit, you'll notice they all scroll the overflow:hidden area on that page during a find for "mozilla".

Given that, as smaug suggested in comment 17, it seems like maybe this bug should get some input from the ux team.

Comment 20

[Elbart]

Another example:
http://www.tomshardware.co.uk/answers/id-2006267/debugging-crash.html

Searching for
> FAILURE_ID_HASH_STRING

Firefox scrolls down to the bottom of the page, where the phrase would show up, if the first posting was fully visible, which it isn't. But currently it's scrolling to an area where the search-result is nowhere to be found.

On that page, the phrase can be found twice.
Because of that Firefox jumps between those two results when clicking "Next", but the user can't actually see them. Very confusing.

IE 11 and Chrome 34 scroll the part of the page into view: http://i.imgur.com/XOyBE6Z.jpg

Comment 21

[Elbart]

Are moz-devs not annoyed by this, or why is there no attention to this bug, which is hiding search-results from the user?
More and more sites are using overflow (my latest encounter: text of longer Facebook-postings hidden behind "See more"), and it's not fun looking for Find-results, which are not visible to the user.

Chrome, IE, even the late Opera 12 shows them.

The worst part about this is that there is no visual indication that a search-result is hidden. The user doesn't even know where to look for it.

Comment 22

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

If there's text that's reachable only by scrolling something that's overflow:hidden (which the user can't scroll), the fact that find behaves oddly doesn't seem like the biggest problem -- how many users will actually use find in order to read the text that they can't scroll to?  The far more serious problem seems to be that the author has designed a page with some of the contents hidden.

If they're hidden in a way that's not common across all browsers, it seems worth filing bugs on that.


I still think it's a bad idea to scroll overflow:hidden elements for find when nothing else scrolls them, especially if authors are sometimes using it to hide things that they want to stay hidden (which would mean scrolling to them would hide the contents that the author wanted visible).

Comment 26

[Gingerbread Man]

Another example would be just about any Wikipedia article, for instance
https://en.wikipedia.org/wiki/Permutation

Looking for the word “jump” there currently shows 47 matches, none of which are visible. The first one has overflow:hidden; height:0px. Confusingly, you can scroll to this one with Next/Previous, although it's completely invisible. The remaining 46 have overflow:hidden; height:1px; width:1px; position: absolute; top: -99999px; and you can't scroll to any of them.


As a workaround, users can add the following style either to the Stylish add-on [1] or the userContent.css file [2].

@-moz-document domain("wikipedia.org") {
#jump-to-nav, .cite-accessibility-label { display: none !important; }
}

[1] https://addons.mozilla.org/firefox/addon/stylish/
[2] http://kb.mozillazine.org/UserContent.css

Comment 27

[alhuber1502]

Totally agree with MichaelSmith: page find in areas hidden by overflow:hidden works in ALL major browsers EXCEPT Firefox.  It should definitely be considered a BUG and be fixed.

Another consideration is that very popular jquery/javascript scrolltools (nicescroll etc.) regularly hide content with overflow:hidden, but allow the user to scroll to the sections afterwards through the script.  In this case, user expectation would be completely broken.

Nicescroll works nicely in IE, Safari, Chrome, Opera, but not in Firefox!  Needs fixing.

Comment 28

[alhuber1502]

I didn't mean to suggest that it should be fixed because Nicescroll isn't working in the last sentence of my previous comment, BTW, I was merely suggesting that users coming from or also using other browsers would be confused by Firefox's behaviour.  Thanks!

Comment 30

[Flore Allemandou [:flore]]

Attachment: bug search in page scrolled.png

I just encountered this bug, with a overflow: hidden for a list.
I wanted to highlight all occurrences of a word and then scroll the list to check the boxes corresponding to the lines where this word was found.
I was first surprised that there were highlights everywhere, but figured it was because it was hidden. OK for me.
Then I scrolled (user voluntary action) and the highlight boxes stayed in place and the text scrolled underneath. Being totally useless. And that's not OK for me.

I wish it would just scroll with the text. Hope that it can be fixed somehow.

Comment 31

[arni2033]

(In reply to Flore Allemandou [:flore] from comment #30)
> Then I scrolled (user voluntary action) and the highlight boxes stayed in place and the text
> scrolled underneath. Being totally useless. And that's not OK for me.
Just to be clear, this bug is about scrolling found string into view when findbar highlights it

> I wish it would just scroll with the text. Hope that it can be fixed somehow.
If you scrolled via mouse wheel / scrollbar, then the bug you're describing is most likely one of connected bugs in bug 384458 (a new ugly findbar that is under development)

Comment 32

[Jonathan Watt [:jwatt]]

Mass bug change to replace various 'parity' whiteboard flags with the new canonical keywords. (See bug 1443764 comment 13.)

Comment 33

[Djfe]

From the bug report Mark linked:
I observed the same behavior on Mozilla's Pontoon a year ago.
my screencapture from back then shows why find's behavior is confusing to the user:
https://www.youtube.com/watch?v=P5Kfs04zGsM

And someone else just commented on that bug report:
"Hi,
we experience the same problem in a medical center.
We have 100+ patient names in a column.
But Ctrl + F in Firefox won't find the name of patient if his name is off screen (no problem with Chromium)."

Comment 34

[BugBot [:suhaib / :marco/ :calixte]]

The severity field for this bug is relatively low, S3. However, the bug has 6 duplicates and 6 See Also bugs.
:enndeakin, could you consider increasing the bug severity?

For more information, please visit auto_nag documentation.

Comment 35

[BugBot (nomail) [:suhaib / :marco/ :calixte]]

The last needinfo from me was triggered in error by recent activity on the bug. I'm clearing the needinfo since this is a very old bug and I don't know if it's still relevant.