Scrolling with selection can leave turds

RESOLVED FIXED in mozilla0.9.1

Status

()

P2
normal
RESOLVED FIXED
19 years ago
17 years ago

People

(Reporter: conrad, Assigned: sfraser_bugs)

Tracking

({platform-parity})

Trunk
mozilla0.9.1
PowerPC
Mac System 8.6
platform-parity
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fix in hand, waiting for tree to open[nsbeta3-][p:2][Mac issue][rtm-], URL)

Attachments

(3 attachments)

(Reporter)

Description

19 years ago
When a text selection is set, replacing another selection, and the content is 
scrolled, the selection that got collapsed is not redrawn and the new selection 
may not be redrawn.

Steps To Reproduce:
1. Go to the above URL.
2. Search for the word "Editor."
3. Hit "Find Next" several times.
4. You will see the problem.
It may depend on the size of your browser window. Mine is the default size on 
the Mac. Basically, if the old selection and new selection are fairly close 
together, and the view has to be scrolled a little to expose the new selection, 
you will see it.

Where to find the problem:
In nsFindComponent.cpp, in nsFindComponent::Context::DoFind(), you will see a 
call to nsITextServicesDocument::SetSelection followed by a call to 
nsITextServicesDocument::ScrollSelectionIntoView.

What I think it is:
When SetSelection() is called, the old selection is collapsed and gets 
invalidated. Also, the new selection gets set and thus invalidated. If the 
window was not scrolled, both would be redrawn properly. But, the window gets 
scrolled and the invalid areas have now moved. If the invalid region does not 
get moved, the area that gets redrawn is the area that was dirty before the 
content wa

Comment 1

19 years ago
Could not reproduce with 2000-03-13-08-M15 on WinNT: the selection after
scrolling was always the newest match. Could this be Mac-only?
conrad@ingress.com - are you still seeing this problem on recent builds of 
Mozilla?

Gerv
Private mail from reporter:

"This problem shows up with what I just pulled and built tonight. It is
easy to reproduce. ON THE MAC, it happens every time. Again, how to see it:
1. Go to mozilla.org.
2. Size the window so that the bold line that says "Mozilla Does SSL" is the
last visible line.
3. Click right before "Mozilla" on that last visible line.
4. Do a "Find on this page" for "Mozilla."
5. Now, find the next one
You should see what I see: The previous occurrence is still hilited and the
second one is as well. See the original bug report for why this happens on
the Mac."

Confirming.

Gerv
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 4

19 years ago
selection->mjudge
Assignee: brade → mjudge
Component: Editor → Selection

Updated

19 years ago
Target Milestone: --- → M16

Comment 5

19 years ago
kathy can you please look at this. I cant. It all works great on windows. is
this just a mac compositor problem?
Assignee: mjudge → brade

Comment 6

19 years ago
I'm not sure I'm going to get to this bug for M16
(Assignee)

Comment 7

19 years ago
*** Bug 34138 has been marked as a duplicate of this bug. ***

Updated

19 years ago
Status: NEW → ASSIGNED

Comment 8

19 years ago
moving over to M17
Keywords: pp
Target Milestone: M16 → M17

Updated

18 years ago
Keywords: correctness
Target Milestone: M17 → M18

Updated

18 years ago
Keywords: nsbeta3

Comment 9

18 years ago
setting to nsbeta3+
Whiteboard: nsbeta3+
(Assignee)

Updated

18 years ago
Summary: Selection and Scrolling → Scrolling with selection can leave turds

Comment 10

18 years ago
adding the brackets in the status
Whiteboard: nsbeta3+ → [nsbeta3+]

Updated

18 years ago
Priority: P3 → P2
Whiteboard: [nsbeta3+] → [nsbeta3+][p:2]

Updated

18 years ago
Assignee: brade → sfraser
Severity: minor → normal
Status: ASSIGNED → NEW

Comment 11

18 years ago
reassign to sfraser since I'm leaving on sabbatical; sorry I didn't get to this 
yet
(Assignee)

Comment 12

18 years ago
P3
Status: NEW → ASSIGNED
Priority: P2 → P3
Whiteboard: [nsbeta3+][p:2] → [nsbeta3+][p:3]
(Assignee)

Comment 13

18 years ago
Conrad, did you have a fix for this a while back?

Comment 14

18 years ago
specific to mac
Whiteboard: [nsbeta3+][p:3] → [nsbeta3+][p:3][Mac issue]
I had a fix in mind but didn't actually try it because I wasn't sure of the 

ramifications. The fix was this - when bits are being scrolled, to move the 

invalid rgn of that port along with the bits. This would mean that if a certain 

area was invalid before the scroll, after the scroll the invalid rgn is moved 

along with it so now the right area will be repainted. Another less general fix 

would be that when unhiliting an old selection to force a redraw of the old 

selection before setting the new selection. 

Comment 16

18 years ago
pdt deems this not necessary for beta3, setting it to p3 which places it below
the wire for beta and rtm, marking future and adding helpwanted. If this impacts
your work or impacts another bug, please state your arguement in the bug, delete
entries in the whiteboard for reconsideration.
Keywords: helpwanted
Target Milestone: M18 → Future

Comment 17

18 years ago
Conrad--my only comment about your proposed solution is that you need to be 
careful to scroll only the portion of the invalidated region in the scrollable 
area (in this case) and not say, in the toolbars, status, over the scrollbars 
themselves, or into the sidebar.

Updated

18 years ago
Keywords: 4xp
(Assignee)

Comment 18

18 years ago
Requesting reconsideration. This causes arrow-key scrolling on the mail thread 
pane to show total garbage; screenshots coming.
Whiteboard: [nsbeta3+][p:3][Mac issue] → [nsbeta3+][Mac issue]
(Assignee)

Comment 19

18 years ago
Created attachment 15223 [details]
Mail thread pane after arrow-key scrolling
(Assignee)

Comment 20

18 years ago
Created attachment 15224 [details]
What the thread pane should have looked like
My suggestion isn't going to change what bits are scrolled. It only shifts the 
invalid rgn on those bits along with them, making sure to clip that region into 
the visible portion of the rect being scrolled.

Updated

18 years ago
Keywords: rtm
Target Milestone: Future → ---

Comment 22

18 years ago
setting status whiteboard so pdt will get it back on their radar
Priority: P3 → P2
Whiteboard: [nsbeta3+][Mac issue] → [nsbeta3+][p:2][Mac issue]
Target Milestone: --- → M19

Comment 23

18 years ago
Not serious enough to hold PR3 off the wire for, so marking nsbeta3-. Probably 
still a good fix to get for RTM though.
Whiteboard: [nsbeta3+][p:2][Mac issue] → [nsbeta3-][p:2][Mac issue]
(Assignee)

Comment 24

18 years ago
Need this for RTM

Comment 25

18 years ago
ok simon, it's up to you to get the stuff in the bug report
Whiteboard: [nsbeta3-][p:2][Mac issue] → [nsbeta3-][p:2][Mac issue][rtm NEED INFO]

Updated

18 years ago
Whiteboard: [nsbeta3-][p:2][Mac issue][rtm NEED INFO] → [nsbeta3-][p:2][Mac issue][rtm+ NEED INFO]

Comment 26

18 years ago
Marking rtm-.
Whiteboard: [nsbeta3-][p:2][Mac issue][rtm+ NEED INFO] → [nsbeta3-][p:2][Mac issue][rtm-]

Comment 27

18 years ago
although scrolling is hideous in the list messages pane, it does not inhibit the
user from using mail, the 'strobe light' effect on scrolling should keep users
amused until the next release -- setting to future
Target Milestone: M19 → Future
(Assignee)

Comment 28

18 years ago
This messes up Find to the extent that it becomes useless, so needs fixing.
Target Milestone: Future → mozilla0.9.1
(Assignee)

Comment 29

18 years ago
Created attachment 33273 [details] [diff] [review]
Fix in nsWindow.cpp
(Assignee)

Comment 30

18 years ago
The attached fix does pretty much what conrad described; when Scrolling the 
window contents, offset any dirty region inside the scrolled frame, and 
invalidate it.
(Assignee)

Updated

18 years ago
Keywords: helpwanted → patch
(Assignee)

Comment 31

18 years ago
reviews please?
Applied the patch and it works well. Only question is - it looks like we're
operating directly on the window's regions, or at least the update rgn. Would it
require less #ifdef'ing for Carbon if we used high level routines like
ValidWindowRgn and InvalWindowRgn. There might be too big a performance hit.

Even without this, r=ccarlen.
(Assignee)

Comment 33

18 years ago
We don't act directly on the window's updateRgn, but I agree that this needs to 
be cleaned up for Carbon.
i have a bug in my list to clean up the target_carbon side to use api's for 
scrolling present in carbon 1.02.

http://bugzilla.mozilla.org/show_bug.cgi?id=35534

while you're mucking around in here, maybe you can inherit that one as well.

Updated

18 years ago
Whiteboard: [nsbeta3-][p:2][Mac issue][rtm-] → fix in hand, awaiting sr [nsbeta3-][p:2][Mac issue][rtm-]

Comment 35

18 years ago
Looks good assuming the (better calculated) update region is subsequently
refreshed or else invalidated, which I assume it is, else scrolling wouldn't
work at all.  sr=scc

Updated

18 years ago
Whiteboard: fix in hand, awaiting sr [nsbeta3-][p:2][Mac issue][rtm-] → fix in hand, waiting for tree to open[nsbeta3-][p:2][Mac issue][rtm-]
(Assignee)

Comment 36

18 years ago
Checked in. Yes, scc, the update region will be updated, since we leave it dirty; 
we'll get an updateEvt for it.
Status: ASSIGNED → RESOLVED
Last Resolved: 18 years ago
Resolution: --- → FIXED

Comment 37

17 years ago
over to tpreston, qa_contact for selection.
QA Contact: sujay → tpreston
You need to log in before you can comment on or make changes to this bug.