Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: zelos full block fields rendering badly #7490

Merged
merged 8 commits into from
Sep 14, 2023
Merged

fix: zelos full block fields rendering badly #7490

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
f81df40
fix: text input fields
BeksOmega Sep 13, 2023
a610595
fix: colour field
BeksOmega Sep 14, 2023
f2a7122
chore: cleanup override keywords
BeksOmega Sep 14, 2023
fa9cc6c
chore: revert playground changes
BeksOmega Sep 14, 2023
67b4464
fix: applyColour not being called
BeksOmega Sep 14, 2023
a0a6c1e
chore: swap visibility for display
BeksOmega Sep 14, 2023
a6b9f8a
chore: cleanup for PR comments
BeksOmega Sep 14, 2023
faeb677
fix: PR comments
BeksOmega Sep 14, 2023
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
19 changes: 19 additions & 0 deletions core/block.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -939,6 +939,25 @@ export class Block implements IASTNodeLocation, IDeletable {
return this.disposed;
}

/**
* @returns True if this block is a value block with a single editable field.
* @internal
*/
isSimpleReporter(): boolean {
if (!this.outputConnection) return false;

let nFields = 0;
for (const input of this.inputList) {
if (input.connection) return false;
// TODO: This comment is innacurate. Not sure what we want the spec to be.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you could just fix the comment. i assume by "text" it means "label" but it doesn't actually make sense to exclude those? like the full block rendering wouldn't make sense if there was a label. so i'd just update the comment.

// Count the number of fields excluding text fields.
for (const _ of input.fieldRow) {
nFields++;
}
}
return nFields <= 1;
}

/**
* Find the connection on this block that corresponds to the given connection
* on the other block.
Expand Down
22 changes: 17 additions & 5 deletions core/field.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -330,6 +330,18 @@ export abstract class Field<T = any>
*/
initModel() {}

/**
* Defines whether this field should take up the full block or not.
*
* Be cautious when overriding this function. It may not work as you expect /
* intend because the behavior was kind of hacked in. If you are thinking
* about overriding this function, post on the forum with your intended
* behavior to see if there's another approach.
*/
protected isFullBlockField(): boolean {
return !this.borderRect_;
}

/**
* Create a field border rect element. Not to be overridden by subclasses.
* Instead modify the result of the function inside initView, or create a
Expand Down Expand Up @@ -797,7 +809,7 @@ export abstract class Field<T = any>
const xOffset =
margin !== undefined
? margin
: this.borderRect_
: !this.isFullBlockField()
? this.getConstants()!.FIELD_BORDER_RECT_X_PADDING
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no action required, but i wish the formatter would scooch these lines over because this stacked ternary operator makes me sad :(

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess the question is: do you want your stacked ternary formatted as if it is:

if (cond1) {
  consequent1;
 } else if (cond2) {
  consequent2;
} else {
  consequent3;
}

or as if it is:

if (cond1) {
  consequent1;
 } else
  if (cond2) {
    consequent2;
  } else {
    consequent3;
  }

I would not say the latter style is particularly popular.

: 0;
let totalWidth = xOffset * 2;
Expand All @@ -813,7 +825,7 @@ export abstract class Field<T = any>
);
totalWidth += contentWidth;
}
if (this.borderRect_) {
if (!this.isFullBlockField()) {
totalHeight = Math.max(totalHeight, constants!.FIELD_BORDER_RECT_HEIGHT);
}

Expand Down Expand Up @@ -922,7 +934,7 @@ export abstract class Field<T = any>
throw new UnattachedFieldError();
}

if (!this.borderRect_) {
if (this.isFullBlockField()) {
// Browsers are inconsistent in what they return for a bounding box.
// - Webkit / Blink: fill-box / object bounding box
// - Gecko: stroke-box
Expand All @@ -940,8 +952,8 @@ export abstract class Field<T = any>
xy.y -= 0.5 * scale;
}
} else {
const bBox = this.borderRect_.getBoundingClientRect();
xy = style.getPageOffset(this.borderRect_);
const bBox = this.borderRect_!.getBoundingClientRect();
xy = style.getPageOffset(this.borderRect_!);
scaledWidth = bBox.width;
scaledHeight = bBox.height;
}
Expand Down
117 changes: 101 additions & 16 deletions core/field_colour.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,12 @@ import * as browserEvents from './browser_events.js';
import * as Css from './css.js';
import * as dom from './utils/dom.js';
import * as dropDownDiv from './dropdowndiv.js';
import {Field, FieldConfig, FieldValidator} from './field.js';
import {
Field,
FieldConfig,
FieldValidator,
UnattachedFieldError,
} from './field.js';
import * as fieldRegistry from './field_registry.js';
import * as aria from './utils/aria.js';
import * as colour from './utils/colour.js';
Expand Down Expand Up @@ -168,29 +173,109 @@ export class FieldColour extends Field<string> {
this.getConstants()!.FIELD_COLOUR_DEFAULT_WIDTH,
this.getConstants()!.FIELD_COLOUR_DEFAULT_HEIGHT,
);
if (!this.getConstants()!.FIELD_COLOUR_FULL_BLOCK) {
this.createBorderRect_();
this.getBorderRect().style['fillOpacity'] = '1';
} else if (this.sourceBlock_ instanceof BlockSvg) {
this.clickTarget_ = this.sourceBlock_.getSvgRoot();
this.createBorderRect_();
this.getBorderRect().style['fillOpacity'] = '1';
this.getBorderRect().setAttribute('stroke', '#fff');
if (this.isFullBlockField()) {
this.clickTarget_ = (this.sourceBlock_ as BlockSvg).getSvgRoot();
}
}

protected override isFullBlockField(): boolean {
const block = this.getSourceBlock();
if (!block) throw new UnattachedFieldError();

const constants = this.getConstants();
return block.isSimpleReporter() && !!constants?.FIELD_COLOUR_FULL_BLOCK;
}

/**
* Updates text field to match the colour/style of the block.
*/
override applyColour() {
if (!this.getConstants()!.FIELD_COLOUR_FULL_BLOCK) {
if (this.borderRect_) {
this.borderRect_.style.fill = this.getValue() as string;
}
} else if (this.sourceBlock_ instanceof BlockSvg) {
this.sourceBlock_.pathObject.svgPath.setAttribute(
'fill',
this.getValue() as string,
);
this.sourceBlock_.pathObject.svgPath.setAttribute('stroke', '#fff');
const block = this.getSourceBlock() as BlockSvg | null;
if (!block) throw new UnattachedFieldError();

if (!this.fieldGroup_) return;

if (!this.isFullBlockField() && this.borderRect_) {
this.borderRect_!.style.visibility = 'visible';
this.borderRect_.style.fill = this.getValue() as string;
} else {
BeksOmega marked this conversation as resolved.
Show resolved Hide resolved
this.borderRect_!.style.visibility = 'hidden';
// In general, do *not* let fields control the color of blocks. Having the
// field control the color is unexpected, and could have performance
// impacts.
block.pathObject.svgPath.setAttribute('fill', this.getValue() as string);
block.pathObject.svgPath.setAttribute('stroke', '#fff');
}
}

/**
* Returns the height and width of the field.
*
* This should *in general* be the only place render_ gets called from.
*
* @returns Height and width.
*/
override getSize(): Size {
if (this.getConstants()?.FIELD_COLOUR_FULL_BLOCK) {
BeksOmega marked this conversation as resolved.
Show resolved Hide resolved
// Full block fields have more control of the block than they should
// (i.e. updating fill colour) so they always need to be rerendered.
this.render_();
this.isDirty_ = false;
}
return super.getSize();
}

/**
* Updates the colour of the block to reflect whether this is a full
* block field or not.
*/
protected override render_() {
super.render_();

const block = this.getSourceBlock() as BlockSvg | null;
if (!block) throw new UnattachedFieldError();
// In general, do *not* let fields control the color of blocks. Having the
// field control the color is unexpected, and could have performance
// impacts.
// Whenever we render, the field may no longer be a full-block-field so
// we need to update the colour.
if (this.getConstants()!.FIELD_COLOUR_FULL_BLOCK) block.applyColour();
}

/**
* Updates the size of the field based on whether it is a full block field
* or not.
*
* @param margin margin to use when positioning the field.
*/
protected updateSize_(margin?: number) {
const constants = this.getConstants();
const xOffset =
margin !== undefined
? margin
: !this.isFullBlockField()
? constants!.FIELD_BORDER_RECT_X_PADDING
: 0;
let totalWidth = xOffset * 2;
let contentWidth = 0;
if (!this.isFullBlockField()) {
contentWidth = constants!.FIELD_COLOUR_DEFAULT_WIDTH;
totalWidth += contentWidth;
}

let totalHeight = constants!.FIELD_TEXT_HEIGHT;
if (!this.isFullBlockField()) {
totalHeight = Math.max(totalHeight, constants!.FIELD_BORDER_RECT_HEIGHT);
}

this.size_.height = totalHeight;
this.size_.width = totalWidth;

this.positionTextElement_(xOffset, contentWidth);
this.positionBorderRect_();
}

/**
Expand Down
94 changes: 57 additions & 37 deletions core/field_input.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ import * as userAgent from './utils/useragent.js';
import * as WidgetDiv from './widgetdiv.js';
import type {WorkspaceSvg} from './workspace_svg.js';
import * as renderManagement from './render_management.js';
import {Size} from './utils/size.js';

/**
* Supported types for FieldInput subclasses.
Expand Down Expand Up @@ -88,7 +89,7 @@ export abstract class FieldInput<T extends InputTypes> extends Field<
* Whether the field should consider the whole parent block to be its click
* target.
*/
fullBlockClickTarget_: boolean | null = false;
fullBlockClickTarget_: boolean = false;

/** The workspace that this field belongs to. */
protected workspace_: WorkspaceSvg | null = null;
Expand Down Expand Up @@ -142,37 +143,22 @@ export abstract class FieldInput<T extends InputTypes> extends Field<

override initView() {
const block = this.getSourceBlock();
if (!block) {
throw new UnattachedFieldError();
}
if (this.getConstants()!.FULL_BLOCK_FIELDS) {
// Step one: figure out if this is the only field on this block.
// Rendering is quite different in that case.
let nFields = 0;
let nConnections = 0;
// Count the number of fields, excluding text fields
for (let i = 0, input; (input = block.inputList[i]); i++) {
for (let j = 0; input.fieldRow[j]; j++) {
nFields++;
}
if (input.connection) {
nConnections++;
}
}
// The special case is when this is the only non-label field on the block
// and it has an output but no inputs.
this.fullBlockClickTarget_ =
nFields <= 1 && block.outputConnection && !nConnections;
} else {
this.fullBlockClickTarget_ = false;
}
if (!block) throw new UnattachedFieldError();
super.initView();

if (this.fullBlockClickTarget_) {
if (this.isFullBlockField()) {
this.clickTarget_ = (this.sourceBlock_ as BlockSvg).getSvgRoot();
} else {
this.createBorderRect_();
}
this.createTextElement_();
}

protected override isFullBlockField(): boolean {
const block = this.getSourceBlock();
if (!block) throw new UnattachedFieldError();

// Side effect for backwards compatibility.
BeksOmega marked this conversation as resolved.
Show resolved Hide resolved
this.fullBlockClickTarget_ =
!!this.getConstants()?.FULL_BLOCK_FIELDS && block.isSimpleReporter();
return this.fullBlockClickTarget_;
}

/**
Expand Down Expand Up @@ -223,23 +209,50 @@ export abstract class FieldInput<T extends InputTypes> extends Field<
* Updates text field to match the colour/style of the block.
*/
override applyColour() {
if (!this.sourceBlock_ || !this.getConstants()!.FULL_BLOCK_FIELDS) return;
const block = this.getSourceBlock() as BlockSvg | null;
if (!block) throw new UnattachedFieldError();

const source = this.sourceBlock_ as BlockSvg;
if (!this.getConstants()!.FULL_BLOCK_FIELDS) return;
if (!this.fieldGroup_) return;

if (this.borderRect_) {
this.borderRect_.setAttribute('stroke', source.style.colourTertiary);
if (!this.isFullBlockField() && this.borderRect_) {
this.borderRect_!.style.visibility = 'visible';
this.borderRect_.setAttribute('stroke', block.style.colourTertiary);
} else {
source.pathObject.svgPath.setAttribute(
this.borderRect_!.style.visibility = 'hidden';
// In general, do *not* let fields control the color of blocks. Having the
// field control the color is unexpected, and could have performance
// impacts.
block.pathObject.svgPath.setAttribute(
'fill',
this.getConstants()!.FIELD_BORDER_RECT_COLOUR,
);
}
}

/**
* Returns the height and width of the field.
*
* This should *in general* be the only place render_ gets called from.
*
* @returns Height and width.
*/
override getSize(): Size {
if (this.getConstants()?.FULL_BLOCK_FIELDS) {
// Full block fields have more control of the block than they should
// (i.e. updating fill colour) so they always need to be rerendered.
this.render_();
this.isDirty_ = false;
}
return super.getSize();
}

/**
* Updates the colour of the htmlInput given the current validity of the
* field's value.
*
* Also updates the colour of the block to reflect whether this is a full
* block field or not.
*/
protected override render_() {
super.render_();
Expand All @@ -256,6 +269,15 @@ export abstract class FieldInput<T extends InputTypes> extends Field<
aria.setState(htmlInput, aria.State.INVALID, false);
}
}

const block = this.getSourceBlock() as BlockSvg | null;
if (!block) throw new UnattachedFieldError();
// In general, do *not* let fields control the color of blocks. Having the
// field control the color is unexpected, and could have performance
// impacts.
// Whenever we render, the field may no longer be a full-block-field so
// we need to update the colour.
if (this.getConstants()!.FULL_BLOCK_FIELDS) block.applyColour();
}

/**
Expand Down Expand Up @@ -374,7 +396,7 @@ export abstract class FieldInput<T extends InputTypes> extends Field<
htmlInput.style.fontSize = fontSize;
let borderRadius = FieldInput.BORDERRADIUS * scale + 'px';

if (this.fullBlockClickTarget_) {
if (this.isFullBlockField()) {
const bBox = this.getScaledBBox();

// Override border radius.
Expand Down Expand Up @@ -462,8 +484,6 @@ export abstract class FieldInput<T extends InputTypes> extends Field<
* @param _value The new value of the field.
*/
onFinishEditing_(_value: AnyDuringMigration) {}
// NOP by default.
// TODO(#2496): Support people passing a func into the field.

/**
* Bind handlers for user input on the text input field's editor.
Expand Down
Loading