Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -204,3 +204,7 @@ critiqueAI.json

pr-comments*.md
*.stackdump

# Per-developer Claude Code settings (the shared .claude/CLAUDE.md stays tracked).
.claude/settings.json
.claude/settings.local.json
20 changes: 15 additions & 5 deletions src/BloomBrowserUI/bookEdit/StyleEditor/StyleEditor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -578,8 +578,12 @@ export default class StyleEditor {
create: boolean,
documentToUse: Document = document,
): CSSStyleRule | null {
const styleSheet =
this.GetOrCreateUserModifiedStyleSheet(documentToUse);
// When we are only reading (create === false), do not create a userModifiedStyles
// sheet as a side effect: a caller looking up a rule that isn't there should not
// mutate the document. Only create the sheet when we actually intend to add a rule.
const styleSheet = create
? this.GetOrCreateUserModifiedStyleSheet(documentToUse)
: this.FindExistingUserModifiedStyleSheet(documentToUse);
if (styleSheet == null) {
return null;
}
Expand Down Expand Up @@ -685,7 +689,10 @@ export default class StyleEditor {
}
}

public getAudioHiliteProps(styleName: string): {
public getAudioHiliteProps(
styleName: string,
documentToUse: Document = document,
): {
hiliteTextColor: string | undefined;
hiliteBgColor: string;
} {
Expand All @@ -694,9 +701,12 @@ export default class StyleEditor {
// The two should have the same content, so for reading, we only need one.
this.sentenceHiliteRuleSelector,
false,
documentToUse,
);
const hiliteTextColor = sentenceRule?.style?.color;
let hiliteBgColor = sentenceRule?.style?.backgroundColor;
const hiliteTextColor =
sentenceRule?.style?.getPropertyValue("color") || undefined;
let hiliteBgColor =
sentenceRule?.style?.getPropertyValue("background-color");
if (!hiliteBgColor) {
hiliteBgColor = kBloomYellow;
}
Expand Down
94 changes: 78 additions & 16 deletions src/BloomBrowserUI/bookEdit/StyleEditor/StyleEditorSpec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -51,14 +51,12 @@ function ChangeSizeAbsolute(
editor.changeSizeInternal(newSize + "pt", shouldSetDefaultRule);
}

function GetUserModifiedStyleSheet(): any {
function GetUserModifiedStyleSheet(): CSSStyleSheet | undefined {
for (let i = 0; i < document.styleSheets.length; i++) {
if (document.styleSheets[i].title == "userModifiedStyles")
if (document.styleSheets[i].title === "userModifiedStyles")
return <CSSStyleSheet>document.styleSheets[i];
}
// this is not a valid constructor
//return new CSSStyleSheet();
return {};
return undefined;
}

function GetFooStyleRuleFontSize(): number {
Expand All @@ -83,8 +81,8 @@ function ParseRuleForFontSize(ruleText: string): number {
}

function GetRuleForFooStyle(): CSSRule | null {
const x: CSSRuleList = (<CSSStyleSheet>GetUserModifiedStyleSheet())
.cssRules;
const x = GetUserModifiedStyleSheet()?.cssRules;
if (!x) return null;

for (let i = 0; i < x.length; i++) {
if (x[i].cssText.indexOf("foo-style") > -1) {
Expand All @@ -95,8 +93,7 @@ function GetRuleForFooStyle(): CSSRule | null {
}

function GetRuleForNormalStyle(): CSSRule | null {
const x: CSSRuleList = (<CSSStyleSheet>GetUserModifiedStyleSheet())
.cssRules;
const x = GetUserModifiedStyleSheet()?.cssRules;
if (!x) return null;

for (let i = 0; i < x.length; i++) {
Expand All @@ -108,8 +105,7 @@ function GetRuleForNormalStyle(): CSSRule | null {
}

function GetRuleForCoverTitleStyle(): CSSRule | null {
const x: CSSRuleList = (<CSSStyleSheet>GetUserModifiedStyleSheet())
.cssRules;
const x = GetUserModifiedStyleSheet()?.cssRules;
if (!x) return null;
for (let i = 0; i < x.length; i++) {
if (x[i].cssText.indexOf("Title-On-Cover-style") > -1) {
Expand All @@ -128,8 +124,9 @@ function GetCalculatedFontSize(target: string): number {
}

function GetRuleMatchingSelector(selector: string): CSSRule | null {
const x = (<CSSStyleSheet>GetUserModifiedStyleSheet()).cssRules;
const count = 0;
const sheet = GetUserModifiedStyleSheet();
const x = sheet?.cssRules;
if (!x) return null;
for (let i = 0; i < x.length; i++) {
if (x[i].cssText.indexOf(selector) > -1) {
return x[i];
Expand All @@ -139,7 +136,9 @@ function GetRuleMatchingSelector(selector: string): CSSRule | null {
}

function HasRuleMatchingThisSelector(selector: string): boolean {
const x = (<CSSStyleSheet>GetUserModifiedStyleSheet()).cssRules;
const sheet = GetUserModifiedStyleSheet();
const x = sheet?.cssRules;
if (!x) return false;
let count = 0;
for (let i = 0; i < x.length; i++) {
if (x[i].cssText.indexOf(selector) > -1) {
Expand All @@ -150,8 +149,8 @@ function HasRuleMatchingThisSelector(selector: string): boolean {
}

function countFooStyleRules(): number {
const x: CSSRuleList = (<CSSStyleSheet>GetUserModifiedStyleSheet())
.cssRules;
const x = GetUserModifiedStyleSheet()?.cssRules;
if (!x) return 0;

let count = 0;
for (let i = 0; i < x.length; i++) {
Expand Down Expand Up @@ -320,6 +319,69 @@ describe("StyleEditor", () => {
if (rule != null) expect(ParseRuleForFontSize(rule.cssText)).toBe(20);
});

it("putAudioHiliteRulesInDom stores audio highlight legacy properties", () => {
const editor = new StyleEditor(
"file://" + "C:/dev/Bloom/src/BloomBrowserUI/bookEdit",
);

const sentenceSelector = "foo-style span.ui-audioCurrent";
const paddedSentenceSelector =
"foo-style span.ui-audioCurrent > span.ui-enableHighlight";
const paragraphSelector = "foo-style.ui-audioCurrent p";

// sanity check that the rules do not yet exist
expect(HasRuleMatchingThisSelector(sentenceSelector)).toBeFalsy();
expect(HasRuleMatchingThisSelector(paddedSentenceSelector)).toBeFalsy();
expect(HasRuleMatchingThisSelector(paragraphSelector)).toBeFalsy();

editor.putAudioHiliteRulesInDom(
"foo-style",
"rgb(1, 2, 3)",
"rgb(4, 5, 6)",
);

const sentenceRule = GetRuleMatchingSelector(sentenceSelector);
const paddedSentenceRule = GetRuleMatchingSelector(
paddedSentenceSelector,
);
const paragraphRule = GetRuleMatchingSelector(paragraphSelector);

expect(sentenceRule?.cssText).toContain(
"background-color: rgb(4, 5, 6)",
);
expect(sentenceRule?.cssText).toContain("color: rgb(1, 2, 3)");
expect(paddedSentenceRule?.cssText).toContain(
"background-color: rgb(4, 5, 6)",
);
expect(paddedSentenceRule?.cssText).toContain("color: rgb(1, 2, 3)");
expect(paragraphRule?.cssText).toContain(
"background-color: rgb(4, 5, 6)",
);
expect(paragraphRule?.cssText).toContain("color: rgb(1, 2, 3)");
});

it("getAudioHiliteProps reads colors from audio highlight legacy properties", () => {
const editor = new StyleEditor(
"file://" + "C:/dev/Bloom/src/BloomBrowserUI/bookEdit",
);

const origProps = editor.getAudioHiliteProps("foo-style");

expect(origProps?.hiliteTextColor).not.toBe("rgb(1, 2, 3)");
expect(origProps?.hiliteBgColor).not.toBe("rgb(4, 5, 6)");

editor.putAudioHiliteRulesInDom(
"foo-style",
"rgb(1, 2, 3)",
"rgb(4, 5, 6)",
);

const props = editor.getAudioHiliteProps("foo-style");

expect(props.hiliteTextColor).toBe("rgb(1, 2, 3)");
expect(props.hiliteBgColor).toBe("rgb(4, 5, 6)");
});

// Skipped because currently we're running in jsdom. Making use of the existing rule depends on
// getComputedStyle, which jsdom does not support. ChatGpt thinks it also depends on actual
// dom element sizes, which jsdom also does not support. Attempts to polyfill proved difficult.
Expand Down
62 changes: 45 additions & 17 deletions src/BloomBrowserUI/bookEdit/css/editMode.less
Original file line number Diff line number Diff line change
Expand Up @@ -1036,15 +1036,40 @@ even when the div is focused (as long as it is empty).*/
color: @bloom-buff;
}

// BL-11633: Works around a Chromium bug where an element with position:relative and a
// background color makes the caret disappear. The bug can trigger on inline text-formatting
// elements at any level of nesting, so this unsets position on those children wherever they
// appear. Use this (rather than .bl11633-unset-position()) on BLOCK containers such as
// div.bloom-editable and p: those must keep their own position:relative, because the little
// language-code tags and paragraph/line-break markers are positioned relative to them and
// would otherwise jump to the wrong corner.
.bl11633-unset-position-on-children() {
em,
i,
strong,
span[style],
u,
sup {
position: unset;
}
}
// For an INLINE element that is itself background-colored (e.g. a decodable/leveled-reader
// violation span, or the sentence span that is the ::highlight range), also unset position
// on the element itself. Do not use this on block containers - see the note above.
.bl11633-unset-position() {
position: unset;
.bl11633-unset-position-on-children();
}

/*This block handles marking elements that violate decodable book and leveled reader constraints*/
span.sentence-too-long {
background-color: @LeveledReaderViolationColor;
position: unset; // BL-11633, works around Chromium bug
.bl11633-unset-position();
}

span.word-too-long {
background-color: @bloom-lightblue;
position: unset; // BL-11633, works around Chromium bug
.bl11633-unset-position();
}

.page-too-many-words-or-sentences .marginBox {
Expand All @@ -1068,21 +1093,24 @@ span.sight-word {

span.word-not-found {
background-color: @DecodableReaderViolationColor;
position: unset; // BL-11633, works around Chromium bug
.bl11633-unset-position();
}

div.bloom-editable {
// Chrome 105 and later (ie, WebView2) has the :has selector.
// These rules may not need the :has(span) selector, but I
// don't want to risk breaking something. This is enough to
// work around the Chromium bug reported in BL-11633.
span[style]:has(span),
em:has(span),
strong:has(span),
u:has(span),
sup:has(span) {
position: unset; // BL-11633, works around Chromium bug
}
// div.bloom-editable and p are block-level and carry the language-code tags and paragraph/
// line-break markers, which are positioned relative to them, so they must keep their own
// position:relative; only their inline children need position:unset. (In the edit tab the
// current highlight is a ::highlight pseudo-element drawn with text-decoration, not a
// background color, so the block itself does not trigger the BL-11633 caret bug.)
div.bloom-editable,
div.bloom-editable p {
.bl11633-unset-position-on-children();
}

// Sentence spans / highlight segments are inline and are themselves the ::highlight range in
// sentence mode, so they (and their inline children) need position:unset on the element itself.
div.bloom-editable p > .audio-sentence,
div.bloom-editable p > .bloom-highlightSegment {
.bl11633-unset-position();
}

/* We are disabling the "Possible Word" feature at this time.
Expand Down Expand Up @@ -1225,10 +1253,10 @@ body {
// and a background color cause the caret not to show up. Somehow, this
// triggers when a decodable reader error span is nested inside one of these
// spans, even though when DR is active the rules that give these segments
// background color are not active. Nothing depends on thei being position:relative,
// background color are not active. Nothing depends on their being position:relative,
// so it's easiest to just prevent it. See BL-11633 for reproduction steps in Bloom 5.5.
.bloom-highlightSegment {
position: unset;
.bl11633-unset-position();
}

// Don't show text over picture borders, resizing handles, or format buttons unless hovering over the image
Expand Down
Loading