Bug 26364

Summary: Search can find text that's hidden by overflow:hidden
Product: WebKit Reporter: Darin Adler <darin>
Component: TextAssignee: Darin Adler <darin>
Status: RESOLVED FIXED    
Severity: Normal CC: finnur.webkit, mitz
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Mac   
OS: OS X 10.5   
Attachments:
Description Flags
patch mitz: review+

Description Darin Adler 2009-06-12 17:58:34 PDT
The site may be gone by the time you see this, but at http://developer.apple.com/wwdc/sessions/ if you search for "AppKit" you'll find something invisible. It's part of a hidden description.
Comment 1 Darin Adler 2009-06-12 17:59:53 PDT
<rdar://problem/6952081>
Comment 2 Darin Adler 2009-06-13 07:55:01 PDT
Created attachment 31236 [details]
patch
Comment 3 mitz 2009-06-13 10:23:11 PDT
Comment on attachment 31236 [details]
patch

> +static inline bool fullyClipsContents(Node* node)
> +{
> +    RenderObject* renderer = node->renderer();
> +    if (!renderer || !renderer->isBox())
> +        return false;
> +    RenderStyle* style = renderer->style();
> +    if (style->overflowX() == OVISIBLE || style->overflowY() == OVISIBLE)
> +        return false;
> +    return toRenderBox(renderer)->size().isEmpty();
> +}

I know that the change log says "We can add other cases here later", but why not handle cases like "overflow-x: hidden; width: 0;" and "overflow-y: hidden; height: 0;" now?

> +    // Push true if this node full clips its contents, or if a parent already has fully

Typo: "full clips".
Comment 4 Darin Adler 2009-06-14 11:18:36 PDT
(In reply to comment #3)
> I know that the change log says "We can add other cases here later", but why
> not handle cases like "overflow-x: hidden; width: 0;" and "overflow-y: hidden;
> height: 0;" now?

I tested those cases in the snippet editor, and in both cases no text was visible. That is also the rule the text iterator implements.
Comment 5 mitz 2009-06-14 11:22:41 PDT
Comment on attachment 31236 [details]
patch

(In reply to comment #4)
> (In reply to comment #3)
> > I know that the change log says "We can add other cases here later", but why
> > not handle cases like "overflow-x: hidden; width: 0;" and "overflow-y: hidden;
> > height: 0;" now?
> 
> I tested those cases in the snippet editor, and in both cases no text was
> visible. That is also the rule the text iterator implements.

Oh, I see: setting overflow-x to hidden sets overflow-y to something other than visible, so
+    if (style->overflowX() == OVISIBLE || style->overflowY() == OVISIBLE)
covers this case.
Comment 6 Darin Adler 2009-06-14 15:08:15 PDT
(In reply to comment #5)
> Oh, I see: setting overflow-x to hidden sets overflow-y to something other than
> visible, so
> +    if (style->overflowX() == OVISIBLE || style->overflowY() == OVISIBLE)
> covers this case.

I guess that || should really be an && -- I'll test to make sure that works and land it that way.
Comment 7 Darin Adler 2009-06-14 20:10:21 PDT
http://trac.webkit.org/changeset/44674
Comment 8 Finnur Thorarinsson 2009-06-17 17:16:59 PDT
Darin, I think we may have a problem with this patch...

See: http://code.google.com/p/chromium/issues/detail?id=14491
Comment 9 Darin Adler 2009-06-17 17:56:53 PDT
(In reply to comment #8)
> Darin, I think we may have a problem with this patch.
> 
> See: http://code.google.com/p/chromium/issues/detail?id=14491

I'd be happy to help fix it. If you can come up with a test case, feel free to put the bug in bugs.webkit.org and assign it to hem.
Comment 10 Finnur Thorarinsson 2009-06-19 12:46:32 PDT
Filed: https://bugs.webkit.org/show_bug.cgi?id=26557
If you need anything more, don't hesitate to contact me.