Vaclav Dvorak Reporter
	Description • 21 years ago

The boolean preference layout.word_select.eat_space_to_next_word isn't settable
in the user's profile (prefs.js or user.js) because of a bug very similar to bug
232321. (Also, for the same reason the pref can't be changed in about:config.)
Patch coming up, hopefully.

	Vaclav Dvorak Reporter
	Comment 2 • 21 years ago

Comment on attachment 143686 [details] [diff] [review]
patch, adapted from bug 232321's attachment 141174 [details] [diff] [review]

Warning: patch by clueless newbie. OTOH, it's all stolen from bug 232321, so it
could be right.

BTW, cute bug of Buzilla in previous comment. :-)

	Boris Zbarsky [:bzbarsky]
	Comment 3 • 21 years ago

I'll try to take a look Saturday morning.

	Boris Zbarsky [:bzbarsky]
	Comment 4 • 21 years ago

Comment on attachment 143686 [details] [diff] [review]
patch, adapted from bug 232321's attachment 141174 [details] [diff] [review]

>Index: layout/html/base/src/nsTextFrame.cpp
>+nsTextFrame::WordSelectListener::Observe(nsISupports *aSubject,
>+                                                 const char *aTopic,
>+                                                 const PRUnichar *aData)

Fix the indent there.

> nsTextFrame::~nsTextFrame()
> {
>+  NS_IF_RELEASE(sWordSelectListener);

Er, no.  That's no good at all.  That will wipe out the listener any time a
text frame is destroyed.  We don't want that.

If you need to add a Shutdown() method of some sort that will get called at app
shutdown, go ahead and do so.

	Vaclav Dvorak Reporter
	Comment 5 • 21 years ago

There must be a less invasive way to do this...??

I moved the definition of class nsTextFrame from the .cpp to the .h, and just
enough #include's to make it compile. I added a Shutdown() method to the class.
The warning applies more than before.

	Boris Zbarsky [:bzbarsky]
	Comment 6 • 21 years ago

I won't be able to get to this for at least two weeks....

	Vaclav Dvorak Reporter
	Comment 7 • 21 years ago

Comment on attachment 143916 [details] [diff] [review]
patch, version 2

OK, thanks anyway. Let's try roc then?

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 8 • 21 years ago

Moving nsTextFrame into nsTextFrame.h is unfortunate but I guess it's the right
thing to do.

However, nsAutoIndexBuffer doesn't need to be in nsTextFrame.h. nsTextFrame.h
just needs "struct nsAutoIndexBuffer;" and the actual definition can stay where
it is.

Ditto for TextStyle. The declaration of TextStyle in nsTextFrame.cpp will be
"struct nsTextFrame::TextStyle { ..."

Ditto for TextReflowData.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 9 • 21 years ago

+#include "nsTextTransformer.h"
+#include "nsIRenderingContext.h"
+#include "nsIFontMetrics.h"
+#include "nsILookAndFeel.h"
+#include "nsITextContent.h"

Plus, I think these includes can be left out of nsTextFrame.h and replaced where
necessary by

class nsIRenderingContext;

etc

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 10 • 21 years ago

+#define TEXT_BUF_SIZE 100

This can stay in nsTextFrame.cpp once nsAutoIndexBuffer is back in
nsTextFrame.cpp ... you get the idea.

	Vaclav Dvorak Reporter
	Comment 11 • 21 years ago

This should do it. As long as I was meddling with this file, I also took the
liberty of correcting some whitespace inconsistencies and tab abuse. It appears
that at various times, the file was edited by people using tabs 8, 2, and even
4 spaces wide. I'll attach a diff -uw version that should be more readable.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 13 • 21 years ago

+class nsFrame;

You need to #include "nsFrame.h" here because it's used as a superclass.

Did this compile?

	Vaclav Dvorak Reporter
	Comment 14 • 21 years ago

Oops. It compiled only because the only files that include nsTextFrame.h also
include nsFrame.h. Thanks, Robert.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 16 • 21 years ago

Comment on attachment 144045 [details] [diff] [review]
patch v4, addressing roc's comment

looks ok to me!

	Vaclav Dvorak Reporter
	Comment 17 • 21 years ago

Robert, could you perhaps recommend a better choice for a reviewer? Thanks.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 18 • 21 years ago

dbaron is the right choice for reviewer.

	David Baron :dbaron:
	Comment 19 • 21 years ago

Comment on attachment 144045 [details] [diff] [review]
patch v4, addressing roc's comment

I haven't seen any good justification for why we want this to be fixed, and it
doesn't seem like it's worth this much code.

However, it could probably be done in much less code with the prefs stuff jst
landed a few days ago (with one big pref observer for all of content/layout). 
Perhaps with that it would be little enough code that the tiny benefit of
allowing users to set this (rather than just following platform conventions,
which is what it's for now) offsets the amount of code increase.

	Ginn Chen Assignee
	Comment 20 • 20 years ago

smaller patch, just copy from bug 232321

	Ginn Chen Assignee
	Comment 21 • 20 years ago

Comment on attachment 158300 [details] [diff] [review]
smaller patch

oops, it's just duplicate of patch version 1. I'm sorry.

	Ginn Chen Assignee
	Comment 22 • 20 years ago

What about move it into nsEventStateManager?
add,
PRBool nsEventStateManager::GetWordSelectEatSpaceAfter()
void nsEventStateManager::ResetWordSelectEatSpaceAfter()

So we don't need add nsTextFrame.h.
And I think nsFrame::GetFrameFromDirection need this setting.
I have a patch for bug 256252, which has this requirement.
I just workaround it, change aPos->mEatingWS to PR_FALSE and change it back 
after calling nsFrame::GetFrameFromDirection. It's a little ugly.

	Ginn Chen Assignee
	Comment 23 • 20 years ago

David, it is important for blind people to have this setting on linux.
For blind people, they need ctrl+right to go to beginning of the next word.
Because, if cursor goes to end of the word, the screen reader may not work 
correctly.

We must get it fixed to make Mozilla accessible on linux.

	David Baron :dbaron:
	Comment 24 • 20 years ago

Comment on attachment 158300 [details] [diff] [review]
smaller patch

you definitely don't want to destroy this observer every time a text frame is
destroyed

	Ginn Chen Assignee
	Comment 25 • 20 years ago

add GetWordSelectEatSpaceAfter, GetWordSelectStopAtPunctuation to
nsTextTransformer

	David Baron :dbaron:
	Comment 26 • 20 years ago

Comment on attachment 158360 [details] [diff] [review]
patch, use nsTextTransformer's PrefCallback

>+PRBool nsTextTransformer::sWordSelectEatSpaceAfter = PR_FALSE;

This should default to true for consistency with the current code.


Also, nsTextTransformer::Shutdown should remove both pref callbacks.

	Ginn Chen Assignee
	Comment 27 • 20 years ago

(In reply to comment #26)
> (From update of attachment 158360 [details] [diff] [review])
> >+PRBool nsTextTransformer::sWordSelectEatSpaceAfter = PR_FALSE;
> This should default to true for consistency with the current code.
On unix, the default values for these two settings are PR_FALSE;
On windows, they're PR_TRUE;
I tested on windows, it works well.
I think we don't want to set default value of sWordSelectEatSpaceAfter to 
PR_TRU, but set sWordSelectStopAtPunctuation to PR_FALSE.

> Also, nsTextTransformer::Shutdown should remove both pref callbacks.
In bug 240543. Hiding some nsIPref* API bloatyness in nsContentUtils.
"NS_IF_RELEASE(sWordSelectListener);" was removed from ShutDown();
Do we still need it?

	David Baron :dbaron:
	Comment 29 • 20 years ago

Comment on attachment 158389 [details] [diff] [review]
patch, add UnregisterPrefCallback

I was suggesting leaving the default value in case something somewhere relied
on what happened when some platform-specific part of all.js didn't set the
pref.

	Kyle Yuan
	Comment 30 • 20 years ago

fixed.