Closed Bug 1629604 Opened 5 years ago Closed 4 years ago

Find in this page: Phrase not found in visible (display:block) <script>, <head>

Categories

(Core :: Find Backend, defect, P5)

Product:
Core
Component:
Find Backend
Version:
75 Branch
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla78
Tracking Flags:
Tracking Status
firefox75 --- wontfix
firefox76 --- wontfix
firefox77 --- wontfix
firefox78 --- fixed

People

(Reporter: sergeykish, Assigned: A-NEUN, Mentored)

References

(Blocks 1 open bug)

Details

(Keywords: good-first-bug, Whiteboard: [lang=c++])

Attachments

(2 files, 1 obsolete file)

find-text-in-head-script-test-color.html
532 bytes, text/html
Details
Bug 1629604 - Fixed phrase not found in visible <script> and added a test for visible <script>. r=emilio
47 bytes, text/x-phabricator-request
Details | Review
Attached file find-text-in-title-script-test.html (obsolete) —

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:75.0) Gecko/20100101 Firefox/75.0

Steps to reproduce:

I've displayed <title> and <script> with CSS

head, title,script, style { display: block }

And searched for text in these elements

Actual results:

"Phrase not found" in <title> and <script>.
Found in <p> and <style>.

Expected results:

It should highlight text that I search

Bugbug thinks this bug should belong to this component, but please revert this change in case of error.

Component: Untriaged → Find Toolbar
Product: Firefox → Toolkit

Added <style> inside <head> (is not searchable).
Added colours to highlight structure

Attachment #9140182 - Attachment is obsolete: true
Component: Find Toolbar → Find Backend
Product: Toolkit → Core

Resetting severity to default of --.

The priority flag is not set for this bug.
:mikedeboer, could you have a look please?

For more information, please visit auto_nag documentation.

Flags: needinfo?(mdeboer)

Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:76.0) Gecko/20100101 Firefox/76.0

Reproduced

Status: UNCONFIRMED → NEW
status-firefox75: --- → affected
status-firefox76: --- → affected
Ever confirmed: true

This is the relevant check. I don't think script should really be there, the IsDisplayedNode check should be more than enough.

So the patch is trivial, and the test should be in test_find.html and should be pretty easy to add too.

Mentor: emilio
Keywords: good-first-bug
Priority: -- → P5
Whiteboard: [lang=c++]
Assignee: nobody → a.neun
Status: NEW → ASSIGNED

Finding within a visible <script> works now, but <head> still does not.
I also had some trouble creating a test for it, as </script>, even within a string, terminates the script running within the test.

(In reply to Tilden Windsor [:A-NEUN] from comment #8)

Finding within a visible <script> works now, but <head> still does not.
I also had some trouble creating a test for it, as </script>, even within a string, terminates the script running within the test.

You can use "</" + "script>" or such instead :)

Thanks! I will try that now.

status-firefox77: --- → affected
Attachment #9149919 - Attachment description: Bug 1629604 - Fixed phrase not found in visible <script>. r=emilio → Bug 1629604 - Fixed phrase not found in visible <script> and added a test for visible <script>. r=emilio

Done.

(In reply to Tilden Windsor [:A-NEUN] from comment #8)

Finding within a visible <script> works now, but <head> still does not.
I also had some trouble creating a test for it, as </script>, even within a string, terminates the script running within the test.

https://searchfox.org/mozilla-central/rev/7fd1c1c34923ece7ad8c822bee062dd0491d64dc/toolkit/components/find/nsFind.cpp#336
https://searchfox.org/mozilla-central/source/accessible/xpcom/xpcAccessibleTextRange.cpp#36

I believe it has something to do with Range.
Just checked - Ctrl+A on the sample page - it selects only <body>

getSelection()
//Selection { anchorNode: body, anchorOffset: 0, focusNode: body, focusOffset: 5, isCollapsed: false, rangeCount: 1, type: "Range", caretBidiLevel: 0 }

That's not the right place in the code, but you're right that at least for find-in-page we do limit results to the <body>: https://searchfox.org/mozilla-central/rev/9193635dca8cfdcb68f114306194ffc860456044/toolkit/components/typeaheadfind/nsTypeAheadFind.cpp#748-750

I don't know why that code is really there. Seems to go back to when the findbar was first introduced. I think we should use GetRootElement unconditionally. Probably separate patch though, with separate test.

That was quick

I've tracked only to mFind->Find() https://searchfox.org/mozilla-central/rev/9193635dca8cfdcb68f114306194ffc860456044/toolkit/components/typeaheadfind/nsTypeAheadFind.cpp#451

So is Ctrl+A is coincidence?

Yes, Ctrl+A is probably a separate one. This one looks suspect.

Could you vouch for my request for tryserver access, as I think it would be useful for checking the tests remotely?

Ah, found it. Should be a separate bug probably.

(In reply to Tilden Windsor [:A-NEUN] from comment #16)

Could you vouch for my request for tryserver access, as I think it would be useful for checking the tests remotely?

Yes, happy to. Please ni? me in your request?

Ok, so there are a couple places that do make assumptions about HTML documents having body contents and so on. It'd be better to change all those in a different bug.

See Also: → editing-outside-body

(In reply to Emilio Cobos Álvarez (:emilio) from comment #17)

Ah, found it. Should be a separate bug probably.

Thank you, https://bugzilla.mozilla.org/show_bug.cgi?id=1638969

Pushed by cbrindusan@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/c41e5eb5a5dd Fixed phrase not found in visible <script> and added a test for visible <script>. r=emilio

https://hg.mozilla.org/mozilla-central/rev/c41e5eb5a5dd

Status: ASSIGNED → RESOLVED
Closed: 4 years ago
status-firefox78: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla78

(In reply to Narcis Beleuzu [:NarcisB] from comment #23)

https://hg.mozilla.org/mozilla-central/rev/c41e5eb5a5dd

I see no fix for <head>... is it resolved?

You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: