Closed
Bug 1366646
Opened 8 years ago
Closed 7 years ago
FinderHighlighter._getRootBounds does not account for border widths or padding in frames - affects modal Find highlighting
Categories
(Toolkit :: Find Toolbar, defect, P1)
Toolkit
Find Toolbar
Tracking
()
VERIFIED
FIXED
mozilla58
| Tracking | Status | |
|---|---|---|
| firefox58 | --- | verified |
People
(Reporter: u462496, Assigned: mikedeboer)
References
Details
Attachments
(3 files, 1 obsolete file)
STR:
1) Load attached file iframe_test.html into tab
2) Set findbar.modalHighlight to `true`
3) Open Findbar
4) Enable "Highlight All"
5) Type the word "frame" into the search textbox
Observe that in the frames with borders, the highlighting is offset to the left, the offset value equaling the border width of the frame, increasing with each level of nesting.
Attachment #8869898 -
Attachment is obsolete: true
Also, padding is not accounted for. Margin seems to be okay.
Summary: FinderHighlighter._getRootBounds does not account for border widths in frames → FinderHighlighter._getRootBounds does not account for border widths or padding in frames - affects modal Find highlighting
Mike, have you seen this bug? I thought I would n'info you about it since IIRC you author on that part of the code.
Flags: needinfo?(mdeboer)
|
Assignee | |
Comment 4•8 years ago
|
||
No, but it sounds like a bug for sure. You're not talking about CSS border and padding, right? Or perhaps that's something we need to add for (i)frames only, because it offsets elements inside them regardless of box model.
Flags: needinfo?(mdeboer)
(In reply to Mike de Boer [:mikedeboer] from comment #4)
> No, but it sounds like a bug for sure. You're not talking about CSS border
> and padding, right? Or perhaps that's something we need to add for (i)frames
> only, because it offsets elements inside them regardless of box model.
Yes, CSS. You can see the attachment.
|
|
Assignee | |
Updated•7 years ago
|
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED
Priority: -- → P1
| Comment hidden (mozreview-request) |
|
||
Comment 7•7 years ago
|
||
| mozreview-review | ||
Comment on attachment 8918912 [details]
Bug 1366646 - Include borders and padding when calculating the offset of a window inside an (i)frame.
https://reviewboard.mozilla.org/r/189804/#review195184
::: toolkit/modules/FinderHighlighter.jsm:632
(Diff revision 1)
> + if (!frameData)
> + dict.frames.set(window, frameData = {});
> + if (frameData.offset)
> + return frameData.offset;
> +
> + let style = frame.ownerGlobal.getComputedStyle(frame);
Can you change the _getFrameElementOffsets method to run async and then at this point instead of doing a sync layout flush, we can use BrowserUtils.promiseLayoutFlushed and get the computed style in the callback?
I noticed that `highlight` is the only async function in FinderHighlighter.jsm. Can/should we make more of the functionality here async?
::: toolkit/modules/tests/browser/browser_FinderHighlighter.js:533
(Diff revision 1)
> await promiseEnterStringIntoFindField(findbar, word);
> await promise;
> });
> });
> +
> +add_task(async function testIframeOffset() {
At some point (perhaps now) we should create a new browser_FinderHighlighter2.js since this one is getting a bit long.
::: toolkit/modules/tests/browser/file_FinderIframeTest.html:1
(Diff revision 1)
> +<!DOCTYPE html>
> +<html>
Can you add a testcase that uses HTML4 and frameborder, marginheight, marginwidth? I don't expect any functional changes to be necessary but it would be good to have a testcase for these attributes.
Attachment #8918912 -
Flags: review?(jaws) → review+
|
|
Assignee | |
Comment 8•7 years ago
|
||
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #7)
> Can you change the _getFrameElementOffsets method to run async and then at
> this point instead of doing a sync layout flush, we can use
> BrowserUtils.promiseLayoutFlushed and get the computed style in the callback?
>
> I noticed that `highlight` is the only async function in
> FinderHighlighter.jsm. Can/should we make more of the functionality here
> async?
Well, yes, with the new tools that we have in our bag, we can make a bunch more things async. But it won't yield us much perf win, I think, because most of the code is already reacting to events and highlighting ranges that come in in chunks.
If I change this single method to become async, I have to update all of its callers and theirs as well. Which will result in a rather big patch... follow up fodder, perhaps?
> At some point (perhaps now) we should create a new
> browser_FinderHighlighter2.js since this one is getting a bit long.
I was thinking that too just yesterday. OK, will fix.
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
|
|
||
Comment 12•7 years ago
|
||
(In reply to Mike de Boer [:mikedeboer] from comment #8)
> (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #7)
> > Can you change the _getFrameElementOffsets method to run async and then at
> > this point instead of doing a sync layout flush, we can use
> > BrowserUtils.promiseLayoutFlushed and get the computed style in the callback?
> >
> > I noticed that `highlight` is the only async function in
> > FinderHighlighter.jsm. Can/should we make more of the functionality here
> > async?
>
> Well, yes, with the new tools that we have in our bag, we can make a bunch
> more things async. But it won't yield us much perf win, I think, because
> most of the code is already reacting to events and highlighting ranges that
> come in in chunks.
>
> If I change this single method to become async, I have to update all of its
> callers and theirs as well. Which will result in a rather big patch...
> follow up fodder, perhaps?
Yeah, follow up fodder is fine. async/await is rather infections (like const in C++). A large patch is somewhat expected.
| Comment hidden (mozreview-request) |
|
||
Comment 14•7 years ago
|
||
Pushed by mdeboer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b036ed8f647a
Include borders and padding when calculating the offset of a window inside an (i)frame. r=jaws
|
||
Comment 15•7 years ago
|
||
| bugherder | ||
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox58:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
|
||
Comment 16•7 years ago
|
||
I can reproduce this issue on Firefox 56.0.2 (20171024165158) under Wind 7 64-bit.
This issue is verified as fixed on Firefox 58.0a1 (2017-11-03) under Wind 7 64-bit and Mac OS X 10.13.
In the frames with borders, the highlighting is not displayed as offset to the left.
Please see the attached video.
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•